ahmarsuhail commented on code in PR #6141:
URL: https://github.com/apache/hadoop/pull/6141#discussion_r1343755412
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/MultipartUtils.java:
##########
@@ -122,9 +122,17 @@ static class ListingIterator implements
this.s3 = s3;
this.requestFactory = storeContext.getRequestFactory();
this.maxKeys = maxKeys;
- this.prefix = prefix;
this.invoker = storeContext.getInvoker();
this.auditSpan = storeContext.getActiveAuditSpan();
+ String p;
+ if (prefix == null) {
Review Comment:
would prefer to move this logic out of the constructor. We do something
similar in
[S3AFileSystem.listMultipartUploads](https://github.com/apache/hadoop/blob/trunk/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java#L5231).
Maybe move to a method in S3AUtils and use that in both places.
Do you know why we were doing the prefix logic in
S3AFileSystem.listMultipartUploads and not here?
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java:
##########
@@ -4293,7 +4304,7 @@ protected synchronized void stopAllServices() {
*/
private void checkNotClosed() throws IOException {
if (isClosed) {
- throw new IOException(uri + ": " + E_FS_CLOSED);
+ throw new PathIOException(uri.toString(), E_FS_CLOSED);
Review Comment:
nit: noticed that the java doc for this method is wrong, `Verify that the
input stream is open` .. can be updated to verify that the FS is open
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3ARetryPolicy.java:
##########
@@ -228,8 +229,16 @@ protected Map<Class<? extends Exception>, RetryPolicy>
createExceptionMap() {
// Treated as an immediate failure
policyMap.put(AWSBadRequestException.class, fail);
- // Status 500 error code is also treated as a connectivity problem
- policyMap.put(AWSStatus500Exception.class, connectivityFailure);
+ // Status 5xx error code is an immediate failure
+ // this is sign of a server-side problem, and while
+ // rare in AWS S3, it does happen on third party stores.
+ // (out of disk space, etc).
+ // by the time we get here, the aws sdk will have
+ // already retried.
+ policyMap.put(AWSStatus500Exception.class, fail);
Review Comment:
nit: Can we mention here that this does not include throttling 503s, and
those are handled separately. the `Status 5xx` makes it slightly ambiguous
##########
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/third_party_stores.md:
##########
@@ -0,0 +1,388 @@
+<!---
+ Licensed under the Apache License, Version 2.0 (the "License");
+ you may not use this file except in compliance with the License.
+ You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License. See accompanying LICENSE file.
+-->
+
+# Working with Third-party S3 Stores
+
+The S3A connector works well worth a third-party S3 stores if the following
requirements are met:
+
+* It correctly implements the core S3 REST API, including support for uploads
clothes and the V2 listing API.
Review Comment:
typo on this line, remove `clothes` ?
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/AWSStatus500Exception.java:
##########
@@ -21,13 +21,13 @@
import software.amazon.awssdk.awscore.exception.AwsServiceException;
/**
- * A 500 response came back from a service.
- * This is considered <i>probably</i> retriable, That is, we assume
- * <ol>
- * <li>whatever error happened in the service itself to have happened
- * before the infrastructure committed the operation.</li>
- * <li>Nothing else got through either.</li>
- * </ol>
+ * A 5xx response came back from a service.
Review Comment:
would be good to mention throttling and 501s are handled separately
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AConfiguration.java:
##########
@@ -552,6 +554,7 @@ public void testS3SpecificSignerOverride() throws Exception
{
config.set(SIGNING_ALGORITHM_STS, "CustomSTSSigner");
config.set(AWS_REGION, EU_WEST_1);
+ disableFilesystemCaching(config);
Review Comment:
why do we need to do this here and not in other cases?
##########
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/third_party_stores.md:
##########
@@ -0,0 +1,388 @@
+<!---
+ Licensed under the Apache License, Version 2.0 (the "License");
+ you may not use this file except in compliance with the License.
+ You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License. See accompanying LICENSE file.
+-->
+
+# Working with Third-party S3 Stores
+
+The S3A connector works well worth a third-party S3 stores if the following
requirements are met:
Review Comment:
nit: typo, works well with
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java:
##########
@@ -2888,6 +2898,7 @@ protected S3ListResult listObjects(S3ListRequest request,
trackDurationOfOperation(trackerFactory,
OBJECT_LIST_REQUEST,
() -> {
+ checkNotClosed(); // this listing is done in the thread pool,
it may actually be closed
Review Comment:
is the concern here that since the listing is done async, by the time it
gets here, S3AFS could have been closed? If yes, we could make the comment
clearer:
`// this listing is done in the thread pool, filesystem could be closed`
also nit: move the comment up a line, above the `checkNotClosed()`
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/ErrorHandling.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.fs.s3a.impl;
+
+import java.io.IOException;
+import java.net.ConnectException;
+import java.net.NoRouteToHostException;
+import java.net.UnknownHostException;
+
+/**
Review Comment:
is the goal to eventually move all translation logic here?
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/impl/TestErrorHandling.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.fs.s3a.impl;
+
+import java.io.IOException;
+import java.net.NoRouteToHostException;
+import java.net.UnknownHostException;
+
+import org.junit.Test;
+import software.amazon.awssdk.core.exception.SdkClientException;
+
+import org.apache.hadoop.test.AbstractHadoopTestBase;
+
+import static
org.apache.hadoop.fs.s3a.impl.ErrorHandling.maybeTranslateNetworkException;
+import static org.apache.hadoop.test.LambdaTestUtils.intercept;
+
+/**
+ * Unit tests related to the {@link ErrorHandling} class.
+ */
+public class TestErrorHandling extends AbstractHadoopTestBase {
+
+ /**
+ * Create an sdk exception with the given cause.
+ * @param message error message
+ * @param cause cause
+ * @return a new exception
+ */
+ private static SdkClientException sdkException(
+ String message,
+ Throwable cause) {
+ return SdkClientException.builder()
+ .message(message)
+ .cause(cause)
+ .build();
+ }
+
Review Comment:
can also add a similar test for ConnectException
--
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]
