-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-41365: Add findDataset/getDataset/getDatasetType to Butler API #899
Merged
+583
−39
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
eb89aee
Add DirectButler.get_dataset_type API
timj 73472aa
Add get_dataset_type to RemoteButler
timj 46c9cee
Add DirectButler.find_dataset
timj 8703186
Add Butler.get_dataset
timj 04bd9b5
Add news fragment
timj 2b6ba5a
Check whether remote butler thinks it is writeable
timj b22a5f3
Use data_id/dataset_type in find_dataset and simplify collections
timj 02c54d2
Add support for day_obs/seq_num to Butler.find_dataset
timj 6c120a9
Add storage class conversion to get_dataset and find_dataset
timj adf8600
Remove lifting of CollectionArgType
timj 694614b
Add missing instrument metadata to test yaml
timj 1e3d68c
Add dimension and datastore record retrieval for get_dataset/find_dat…
timj 17bc74e
Add experimental exception handling in client/server
timj File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Added new ``Butler`` APIs migrated from registry: ``Butler.get_dataset_type()``, ``Butler.get_dataset()``, and ``Butler.find_dataset()``. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we provide this as an option, we should also provide a way to expand the data ID (i.e. add dimension records) at the same time, and we should think about whether we want just one option to do both or allow the two kinds of expansion to happen separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had completely failed to notice that that flag was in there now. One option is for
find_dataset
to always expand and add everything. People shouldn't be calling it in bulk.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always expanding could be inefficient, I think many clients do not care about datastore records at all, we should not be pessimizing things for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably get_dataset should have the datastore_records and dimension_records booleans as well for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also need to add datastore records to the SerializedDatasetRef because that was never updated and so it's impossible to return datastore records from the server at the moment...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jim asked me to not add them to
SerializedDatasetRef
, as it may increase graph size on disk (but I don't think we give records to refs that go into the graph, at least not yet).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are sending SerializedDatasetRef over the wire so we can only reconstruct things in the far end that were put in at the other end. We need to add them somehow (but obviously they should be optional). For now I've turned the feature off in client/server so you can't get the datastore records.