DM-42190: Allow use of client/server Butler #125
Merged
Conversation
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
dhirving
force-pushed
the
tickets/DM-42190
branch
7 times, most recently
from
7935f69
to
a17b301
Compare
rra
approved these changes
Jan 23, 2024
requirements/main.in
Outdated
Comment on lines
23
to
30
| safir[gcs] | ||
| #safir[gcs] | ||
| structlog | ||
|
|
||
| # Uncomment this, change the branch, comment out safir above, and run make | ||
| # update-deps-no-hashes to test against an unreleased version of Safir. | ||
| #git+https://github.com/lsst-sqre/safir@main#egg=safir[gcs] | ||
| git+https://github.com/lsst-sqre/safir@main#egg=safir[gcs] |
There was a problem hiding this comment.
A new Safir has now been released with your change.
Upgrade safir for auth_delegated_token_dependency, and daf_butler for LabeledButlerFactory.
Use the new Butler factory class 'LabeledButlerFactory' to construct Butler instances. This has two benefits: 1. The previous construction pattern, where a single Butler instance was shared globally between requests, was not threadsafe. Butler instances have internal state for database connection management and caching that gets confused if used simultaneously from more than one request/thread. LabeledButlerFactory creates a separate Butler instance for each request. 2. The factory allows the use of Butler's client/server mode, instead of always requiring direct access to the database.
The 'MissingCollectionError' issue that this code was working around was fixed a few months ago in DM-41117, with a test added in DM-42138.
The 'registry' object is now deprecated, and the getDataset function was moved to the root Butler interface as 'get_dataset'.
In the next commit I will be adding a usage of another ResourcePath property, so instead of mocking it there is no reason not to use the real library. There was also a convoluted way of specifying the test size that was effectively a constant, so just make it a constant.
Butler server returns signed URLs directly, so there is no need for additional signing.
dhirving
force-pushed
the
tickets/DM-42190
branch
from
a17b301
to
3bdc53c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Updated usage of the Butler to make it compatible with the new client/server version of Butler.
The biggest change is that we now use a new Butler API 'LabeledButlerFactory' for creating the Butler. This adds support for RemoteButler, and also fixes some issues when using the old DirectButler. (The way Datalinker was using a single global Butler shared between threads was never OK -- DirectButler instances are not threadsafe because they have database session state that can get extremely confused if multiple threads use it at the same time.)
Butler server returns signed URLs directly, so there is a new case in the links endpoint that bypasses Datalinker's own URL signing when it gets an HTTP URL from the Butler.
I eliminated usage of some deprecated Butler methods that are not supported by RemoteButler. Also removed a workaround for a Butler bug that was fixed several months ago.
This does not immediately change the deployed Datalinker services to use Butler server -- that will be done via a configuration change in Phalanx in the coming weeks.
This PR depends on unreleased versions of safir, daf_butler, and resources, so this won't merge until those are updated to point to real releases.