Skip to content
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

Implement client direct reads #1166

Merged
merged 5 commits into from
May 3, 2024

Conversation

joereuss12
Copy link
Contributor

@joereuss12 joereuss12 commented Apr 25, 2024

This implements direct read functionality within the client. Adds a query parameter to add to the transfer url ?directread. When this is set, the client will avoid using a cache and go directly to the origin for its downloads. This PR also adds some unit tests for this functionality with a few bug fixes along the way. This should work with both the client and the plugin as well as with the ?pack and ?recursive query.

Closes #516

@joereuss12 joereuss12 added this to the v7.8.0 milestone Apr 25, 2024
@joereuss12 joereuss12 force-pushed the cache-bypass-branch branch 2 times, most recently from 43aed20 to 128519e Compare April 26, 2024 13:52
client/director.go Outdated Show resolved Hide resolved
client/fed_test.go Show resolved Hide resolved
client/fed_test.go Outdated Show resolved Hide resolved
client/fed_test.go Outdated Show resolved Hide resolved
client/fed_test.go Outdated Show resolved Hide resolved
client/main.go Outdated Show resolved Hide resolved
cmd/plugin_test.go Outdated Show resolved Hide resolved
cmd/plugin_test.go Outdated Show resolved Hide resolved
cmd/plugin_test.go Outdated Show resolved Hide resolved
cmd/plugin_test.go Outdated Show resolved Hide resolved
@joereuss12 joereuss12 force-pushed the cache-bypass-branch branch from 8ee552b to a5410b6 Compare April 29, 2024 21:19
@joereuss12 joereuss12 requested a review from jhiemstrawisc May 1, 2024 15:30
client/fed_test.go Show resolved Hide resolved
client/resources/pub-export-no-directread.yml Outdated Show resolved Hide resolved
client/resources/pub-origin-no-directread.yml Outdated Show resolved Hide resolved
utils/utils.go Show resolved Hide resolved
@joereuss12 joereuss12 force-pushed the cache-bypass-branch branch 2 times, most recently from 3362b67 to 7a4aba4 Compare May 2, 2024 21:25
@joereuss12 joereuss12 requested a review from jhiemstrawisc May 3, 2024 12:39
utils/utils.go Outdated Show resolved Hide resolved
utils/utils.go Outdated
if !isPlugin && hasRecursive {
return fmt.Errorf("Cannot use the recursive query parameter when not utilizing the pelican plugin")
return errors.New("cannot use the recursive query parameter when not utilizing the pelican plugin")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, let's say "recursive query parameter not yet supported in URLs outside of plugin" or something to that effect. That tells the user it's coming, but not yet ready.

My second comment is that we need an issue to keep track of this. Can you open an issue and link it to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the issue, I added it to it #1165

The issue includes this PR and I have notes to modify this function as well as the tests. Anything else I need to do?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, thanks for linking it :)

utils/utils.go Outdated Show resolved Hide resolved
joereuss12 added 4 commits May 3, 2024 15:37
This implements direct read functionality within the client. Adds a
query parameter to add to the transfer url ?directread. When this is
set, the client will avoid using a cache and go directly to the origin
for its downloads. This PR also adds some unit tests for this
functionality with a few bug fixes along the way. This should work with
both the client and the plugin as well as with the ?pack and ?recursive
query.
Moved CheckQueryParam to utils instead of in two different packages as
well as moved the test for it. Added more test cases for directread
functionality.
changes:
- Changed tests to instead of look at cache location for if the file
  does not exist, look at the endpoint returned from the transfer
- Modified the checkValidQuery function, fixing up errors returned as
  well as ways to check the query
- Fixed typo for embedding test configs, modified one of the configs to
  work with test by adding another namespace
- Modified comments in test yaml configs
@joereuss12 joereuss12 force-pushed the cache-bypass-branch branch from 7a4aba4 to 31338f2 Compare May 3, 2024 15:50
@joereuss12 joereuss12 requested a review from jhiemstrawisc May 3, 2024 16:19
@joereuss12 joereuss12 force-pushed the cache-bypass-branch branch from 80b36a1 to 2763403 Compare May 3, 2024 17:05
@jhiemstrawisc jhiemstrawisc merged commit 1827884 into PelicanPlatform:main May 3, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow cache bypass based on query parameters
2 participants