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

cross-region-support-cache level changes #93

Merged
merged 3 commits into from
Jan 10, 2024
Merged

Conversation

shiva958
Copy link
Collaborator

@shiva958 shiva958 commented Jan 9, 2024

Issue #, if available:
Currently, the design for the caching layer only accepts a single instance of S3ControlClient during initializations. The problem with this is that, the S3ControlClient is configured for one region and cannot be used for making cross-region requests. Since, S3ControlClients do not have a cross-region setting to redirect requests to the correct region, we will be making headbucket requests to determine the correct region for the bucket and configure the S3ControlClient accordingly.

Description of changes:

  • modifying the cache layers to remove passing S3ControlClients as part of the builder. Control Clients configured to the correct region will instead be passed as an input for the method that will be invoked by the plugin.
  • Adding a Bucket Region cache. This region fetched for the bucket will be cached to avoid throttling S3 head bucket API.
  • Adding and modifying unit tests.

Testing:
This PR is part of a large subset of PR's. Changes have been tested by running

  • Unit-tests
  • Integration-tests
  • Spinning multiple threads in parallel sharing a single instance of s3client to send requests supported by Access Grants.

Coverage:
Cache layer - 86%

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Collaborator

@amitvc amitvc left a comment

Choose a reason for hiding this comment

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

Minor comments. Approved

import software.amazon.awssdk.services.s3control.S3ControlAsyncClient;
import software.amazon.awssdk.services.s3control.model.S3ControlException;

public interface S3AccessGrantsBucketRegionResolver {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Make this interface BucketRegionResolver and the impl S3AccessGrantsBucketRegionResolver.
Make interface names very generic and not specific

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank You. Will do this in the up-coming PR for threading.

@shiva958 shiva958 marked this pull request as ready for review January 10, 2024 18:07
@shiva958 shiva958 requested a review from a team as a code owner January 10, 2024 18:07
@shiva958 shiva958 merged commit 164cfba into main Jan 10, 2024
2 of 6 checks passed
@shiva958 shiva958 deleted the cross-region-support-cache branch January 10, 2024 18:12
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.

3 participants