ahmarsuhail commented on code in PR #6479:
URL: https://github.com/apache/hadoop/pull/6479#discussion_r1471123903
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/DefaultS3ClientFactory.java:
##########
@@ -289,17 +290,36 @@ private <BuilderT extends S3BaseClientBuilder<BuilderT,
ClientT>, ClientT> void
builder.fipsEnabled(fipsEnabled);
if (endpoint != null) {
+ boolean overrideEndpoint = true;
checkArgument(!fipsEnabled,
"%s : %s", ERROR_ENDPOINT_WITH_FIPS, endpoint);
- builder.endpointOverride(endpoint);
- // No region was configured, try to determine it from the endpoint.
- if (region == null) {
- region = getS3RegionFromEndpoint(parameters.getEndpoint());
+ boolean endpointEndsWithCentral =
+ endpointStr.endsWith(CENTRAL_ENDPOINT);
+ // No region was configured or the endpoint is central,
+ // determine the region from the endpoint.
+ if (region == null || endpointEndsWithCentral) {
Review Comment:
hmm, not sure about this. now we're parsing region if region is null or
endpoint = s3.amazonaws.com.
So if you set `s3.amazonaws.com` and region to eu-west-2, you still end up
with us setting the region to `us-east-2` and cross region enabled. My thinking
here is that a lot of people may have endpoint set to s3.amazonaws.com (as
atleast with SDK V1 it was harmless to do that I think) .
we only want to get into this parsing if region == null. so let's revert to
the previous condition here. And then we never don't want to override if the
endpoint is s3.amazonaws.com. Suggested:
```
if (endpoint != null) {
checkArgument(!fipsEnabled,
"%s : %s", ERROR_ENDPOINT_WITH_FIPS, endpoint);
boolean endpointEndsWithCentral =
endpointStr.endsWith(CENTRAL_ENDPOINT);
// No region was configured or the endpoint is central,
// determine the region from the endpoint.
if (region == null) {
region = getS3RegionFromEndpoint(endpointStr,
endpointEndsWithCentral);
if (region != null) {
origin = "endpoint";
if (endpointEndsWithCentral) {
builder.crossRegionAccessEnabled(true);
origin = "origin with cross region access";
LOG.debug("Enabling cross region access for endpoint {}",
endpointStr);
}
}
}
// No need to override endpoint with "s3.amazonaws.com".
// Let the client take care of endpoint resolution. Overriding
// the endpoint with "s3.amazonaws.com" causes 400 Bad Request
// errors for non-existent buckets and objects.
// ref: https://github.com/aws/aws-sdk-java-v2/issues/4846
if (!endpointEndsWithCentral) {
builder.endpointOverride(endpoint);
LOG.debug("Setting endpoint to {}", endpoint);
}
}
```
So now:
1) if endpoint = s3.amazonaws.com and region is null, set to US_EAST_2 and
enable cross region, and don't override endpoint.
2) if endpoint = s3.amazonaw.com and region is set (eg to eu-west-1), set
region but do not override endpoint..let SDK figure it out
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEndpointRegion.java:
##########
@@ -146,7 +150,21 @@ public void testCentralEndpoint() throws Throwable {
describe("Create a client with the central endpoint");
Configuration conf = getConfiguration();
- S3Client client = createS3Client(conf, CENTRAL_ENDPOINT, null, US_EAST_1,
false);
+ S3Client client = createS3Client(conf, CENTRAL_ENDPOINT, null, US_EAST_2,
false);
+
+ expectInterceptorException(client);
+ }
+
+ @Test
+ public void testCentralEndpointWithRegion() throws Throwable {
+ describe("Create a client with the central endpoint but also specify
region");
+ Configuration conf = getConfiguration();
+
+ S3Client client = createS3Client(conf, CENTRAL_ENDPOINT, US_WEST_2,
US_EAST_2, false);
Review Comment:
for example here, if configured region is US_WEST_2, expected region should
also be US_EAST_2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
