make downloader more robust#666
Conversation
|
Before you submit for review:
If you did not complete any of these, then please explain below. |
MarkWolters
left a comment
There was a problem hiding this comment.
approved, but I think we should try to refrain from using hard-coded conventions in the future if possible (see comment).
| if (owners.size() <= 1) continue; | ||
|
|
||
| boolean involvesBaseOrGt = owners.stream() | ||
| .anyMatch(o -> "base".equals(o.facet) || "gt".equals(o.facet)); |
There was a problem hiding this comment.
Would it be possible to make these constants ("base", "gt") configurable? This works for our current naming standards but hardcoding it here enforces a sort of invisible rule that's disconnected from the dataset creation process. I don't think it needs to be changed immediately to merge, but its something we might want to change for future flexibility.
There was a problem hiding this comment.
I've had this come up in a couple places before, and the main issue is that if there is no standard, then we have to wire the facets up everywhere and it lowers the utility of having an access layer altogether.
But what I have done also is allow all common "spellings" of a facet type to be recognized to support cases which are unambiguous.
The same names are used in upstream dataset creation (in some tools) canonically, with one favored as the generally prescribed name. We do need some type of naming consistency for facets that all the tools can agree on. Happy to make a change if we can address these other concerns too.
This does a couple things to make downloads more defensive:
These are defensive changes which simply cause these types of errors to be surfaced. The bias here is to avoid surprises or non-deterministic results.