[
https://issues.apache.org/jira/browse/HADOOP-18889?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17771483#comment-17771483
]
ASF GitHub Bot commented on HADOOP-18889:
-----------------------------------------
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
> S3A: V2 SDK client does not work with third-party store
> -------------------------------------------------------
>
> Key: HADOOP-18889
> URL: https://issues.apache.org/jira/browse/HADOOP-18889
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/s3
> Affects Versions: 3.4.0
> Reporter: Steve Loughran
> Assignee: Steve Loughran
> Priority: Critical
> Labels: pull-request-available
>
> testing against an external store without specifying region now blows up
> because the region is queried off eu-west-1.
> What are we do to here? require the region setting *which wasn't needed
> before? what even region do we provide for third party stores?
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
