S3CrossRegion Sync and Async Clients Redirect implementation

[joviegas]

Motivation and Context

• Implement redirection in S3CrossRegionSyncClient and S3CrossRegionAsyncClient
• Perform a redirect whenever the first call fails with a redirect exception.
• Pass the supplier to endPointProvider to retrieve the region from cache or headBucket call.

Modifications

• The initial call is made using the default client.
• If the call fails with a 301 status error, the region is extracted from the error response header.
• The cache is updated, and the call is made by overriding the endpoint provider.
• The overridden endpoint provider checks the cache to obtain the region.
• If the region is not updated in the cache, a HeadBucket call is made to retrieve and update the region.

Testing

• Junits added

License

• I confirm that this pull request can be released under the Apache 2 license

[cenedhryn]

Note: Failed codegen test should be fixed by PR #4080

[cenedhryn]

In the sync case, the try-catch contains both the call with cache hit and the cache miss, so the catch covers both cases. In this async instance, we will not catch a potential redirect as a result of calling with a cached region and we won't clear the cache?

[joviegas]

Line number 66 clears the cache.
Note that HeadBucket fall back logic comes into picture only when first fall back fails to get region and in this case we clear the cache.

[cenedhryn]

I meant the code within the if-statement if (bucketToRegionCache.containsKey(bucketName)). No other code will execute below that. If the call to requestWithDecoratedEndpointProvider gets a redirect, it will not be handled. So the sync and async code will have slightly different behavior in that edge case. Question is if it's likely to occur at all and therefore worth handling - i.e., you get a region from the cache but it's the wrong region. Probably not a huge risk.

[joviegas]

Good catch !! This can happen if the bucket is changed, thanks for catching this bug.
Will update the code and add a test case for below standalone API call scenarios

1. Redirect Cached and Success ==> 1st call
2. Success ==> 2nd call
3. Redirect and success ==> 3 rd Call

[zoewangg]

Should we use LruCache to prevent this map from growing unbounded?

[joviegas]

LruCache cache doenot provide API to modify the Cache it just caches the content and provides get.
I will create a separate PR to make this bounded cache

[zoewangg]

Maybe we can update LruCache to support this? @cenedhryn WDYT?

[cenedhryn]

It seems most use cases for Lru can be solved with a LinkedHashMap. (It may make sense to rewrite our LruCache on that model, but that's a different question)

For instance:

    private static final int CAPACITY = 100;

    private final Map<String, Region> bucketToRegionCache = Collections.synchronizedMap(
        new LinkedHashMap<String, Region>(CAPACITY) {
            protected boolean removeEldestEntry(Map.Entry eldest) {
                return size() > CAPACITY;
            }
        });

[joviegas]

Thanks , Will make the capacity as 300 to remain consistent with V1

https://github.com/aws/aws-sdk-java/blob/71353f994f0a78efac98c50db4da59d1a8a8cdcc/aws-java-sdk-s3/src/main/java/com/amazonaws/services/s3/AmazonS3Client.java#L480-L490

[joviegas]

Looks like we cannot implement using ConcurrentHashMap since it internally just does a putAlll and doesnot actually use LinkedHashMap in above

[cenedhryn]

Can you also add a @SuppressWarnings("unchecked") statement to requestWithDecoratedEndpointProvider in CrossRegionUtils in that PR?

[joviegas]

Done ...Was wondering if there is a tool/scan which suggest this automatically ?

[cenedhryn]

I usually notice because IntelliJ gets angry and makes lines yellow.

[sonarqubecloud]

SonarCloud Quality Gate failed.   

5 Bugs
0 Vulnerabilities
0 Security Hotspots
74 Code Smells

89.7% Coverage
0.1% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint