Conversation
spackbot-app
bot
added
core
haampie
force-pushed
the
fix/cache-s3-connection
branch
from
964f348 to
59bef1d
Compare
haampie
force-pushed
the
fix/cache-s3-connection
branch
from
59bef1d to
ee6732c
Compare
haampie
force-pushed
the
fix/cache-s3-connection
branch
from
b4e554c to
6a60b3e
Compare
scottwittenburg
left a comment
scottwittenburg
left a comment
There was a problem hiding this comment.
Thanks @haampie! Caching s3 clients is a great idea, and this looks to be a fairly straightforward (let's put that in air quotes though) adaptation of what we had before to support that. I wasn't really familiar with this bit of the code before, so I added some questions to check my understanding. Feel free to ignore them unless you have time to answer.
Otherwise, LGTM. I'm not sure how well the unit tests mock s3 read/write, so maybe I should keep an eye on the pipelines after this is merged 😜
| } | ||
| mirror = spack.mirror.Mirror.from_dict( | ||
| { | ||
| "fetch": { |
There was a problem hiding this comment.
Something I'm confused about is why you changed from an arbitrary dictionary of connection data (mock_connection_data) to this thing that is actually a Mirror. For one thing, my understanding was that the endpoint_url was optional (and only used/needed for S3 buckets that don't live in AWS S3, but rather minio or similar). If this were a real S3 mirror config, would it have another url? I.e. the s3:\\ url?
Also, I'm surprised it's valid to specify all three auth methods simultaneously, I would have thought you'd want to specify only one of access_token, profile, or access_pair. Maybe specifying all three just helps with testing though?
There was a problem hiding this comment.
Many different urls correspond to one (or no) mirror, so I thought it was an easier key to use
|
|
||
| def _s3_open(url): | ||
| parsed = url_util.parse(url) | ||
| s3 = s3_util.create_s3_session(parsed, connection=s3_util.get_mirror_connection(parsed)) |
There was a problem hiding this comment.
Was it a bug that this didn't pass url_type="fetch" previously?
| if url_result.scheme == "s3": | ||
| # Check for URL-specific connection information | ||
| s3 = s3_util.create_s3_session( | ||
| url_result, connection=s3_util.get_mirror_connection(url_result) |
There was a problem hiding this comment.
Is this another case where url_type="fetch" should have been passed before?
| import spack.config | ||
| import spack.util.url as url_util | ||
|
|
||
| #: Map (mirror name, method) tuples to s3 client instances. |
There was a problem hiding this comment.
Based on our conversation in slack, I was expecting a mapping from (url, method) to s3 client instance, rather than (name, method) to client instance.
| mirrors = [ | ||
| (name, mirror) | ||
| for name, mirror in all_mirrors.items() | ||
| if url_str.startswith(get_mirror_url(mirror)) |
There was a problem hiding this comment.
Maybe I get it. This is where you've converted from the provide url to a mirror name, which you've possibly done in support of the future goal of using mirror names (rather than mirror urls) everywhere?
There was a problem hiding this comment.
Actually the old code was already like this, but maybe a comment got lost
|
@spackbot run pipeline |
|
I'm sorry, gitlab does not have your latest revision yet, I can't run that pipeline for you right now. One likely possibility is that your PR pipeline has been temporarily deferred, in which case, it is awaiting a Please check the gitlab commit status message to see if more information is available. DetailsUnexpected response from gitlab: {'message': '404 Commit Not Found'} |
2 participants
Use a cache of the form
for S3 connections, to avoid the overhead of creating it on every request.
With this PR:
Before: