ahmarsuhail commented on code in PR #6479:
URL: https://github.com/apache/hadoop/pull/6479#discussion_r1469449899
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEndpointRegion.java:
##########
@@ -257,6 +275,44 @@ public void testWithVPCE() throws Throwable {
expectInterceptorException(client);
}
+ @Test
Review Comment:
we should also add a test for a key that does not exist, eg:
```
newFS.getObjectMetadata("nonExistentKey.txt");
```
verify that we get a 404 and not a 400.
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/DefaultS3ClientFactory.java:
##########
@@ -289,17 +290,35 @@ 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 =
Review Comment:
so from the response on SDK issue, it looks like we don't want to override
if endpoint is `s3.amazonaws.com` and region is not `US_EAST_1`.
Currently, if `fs.s3a.endpoint` is `s3.amazonaws.com` and
`fs.s3a.endpoint.region` is `eu-west-1` for example, we will end up overriding
and end up with the same 400 errors.
What if we never override if endpoint is s3.amazonaws.com?
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEndpointRegion.java:
##########
@@ -257,6 +275,33 @@ public void testWithVPCE() throws Throwable {
expectInterceptorException(client);
}
+ @Test
+ public void testCentralEndpointCrossRegionAccess() throws Throwable {
+ describe("Create bucket on different region and access it using central
endpoint");
+ Configuration conf = getConfiguration();
+ removeBaseAndBucketOverrides(conf, ENDPOINT, AWS_REGION);
+
+ Configuration newConf = new Configuration(conf);
+
+ newConf.set(ENDPOINT, CENTRAL_ENDPOINT);
+
+ newFS = new S3AFileSystem();
+ newFS.initialize(getFileSystem().getUri(), newConf);
+
+ final String file = getMethodName();
Review Comment:
this can all be simplified by just doing
`newFS.create(methodPath()).close();` the teardown of the tests will also
delete the file created in that case, and you won't have to do the delete call.
Also no need to to actually write anything to the file as all we care about is
if the request is routed to the right place or not.
--
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]
