Server-side download URL generation#1438
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1438 +/- ##
===========================================
+ Coverage 88.91% 89.36% +0.45%
===========================================
Files 103 104 +1
Lines 3679 3921 +242
Branches 454 494 +40
===========================================
+ Hits 3271 3504 +233
- Misses 333 342 +9
Partials 75 75 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ponyisi
force-pushed
the
server-url-generation
branch
from
c75c2e9 to
31f7365
Compare
|
|
||
| class FileURLGenerator(ServiceXResource): | ||
| def __init__(self): | ||
| if True: # S3 backend |
There was a problem hiding this comment.
Why is this conditional block needed? How does S3 hook in to this?
There was a problem hiding this comment.
Sorry, this is admittedly unclear. Basically, this is a placeholder for an if-statement for a different storage backend (e.g. xrootd).
There was a problem hiding this comment.
Can you set a backend class attribute that is typed as a literal and defaults to "s3"? Then it can be used for the two conditional blocks.
There was a problem hiding this comment.
I guess it actually shouldn't be a class attribute but an instance attribute so it's defined when the object is created
There was a problem hiding this comment.
Actually thinking about this further, the backend is a global setting right (i.e. we don't support multiple backends)? Then the backend system should be defined in the helm values and can be used in the conditional here.
But also, when does the __init__ method fire for flask routes like this? I don't see it commonly being used for resources (and there's no super call). Is one object created when the route is setup, or is one object created for every request? If the latter, then this __init__ implementation would generate unnecessary boto objects. Instead, the client could be moved outside FileURLGenerator so one client is used consistently.
There was a problem hiding this comment.
Yes, the intention was that when we actually figure out how we want to signal the backend system being used via the helm chart, that the conditional would work based on that.
The __init__ is, as far as I know, only called when the routes are being set up. I think the call to the superclass constructor is essentially a noop but I'll add it.
2 participants
This is a first pass at generating download URLs for ServiceX output on the server side. The client can request a list of files in a transform, and URLs will be returned.
We have a hook here for doing this with other backends, e.g. xrootd-based ones. At some point we also probably want to cache results for some length of time.