steveloughran commented on code in PR #5881: URL: https://github.com/apache/hadoop/pull/5881#discussion_r1298441035
########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java: ########## @@ -56,6 +56,7 @@ import org.apache.hadoop.classification.VisibleForTesting; import org.apache.hadoop.fs.impl.BackReference; +import org.apache.hadoop.fs.azurebfs.services.StaticRetryPolicy; Review Comment: nit: put above L58 to try and keep imports in order ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRetryPolicy.java: ########## @@ -0,0 +1,73 @@ +/** + * 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.azurebfs.services; + +import java.net.HttpURLConnection; + +import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.HTTP_CONTINUE; + +/** + * Abstract Class for Retry policy to be used by {@link AbfsClient} + * Implementation to be used is based on retry cause. + */ +public abstract class AbfsRetryPolicy { + + /** + * The maximum number of retry attempts. + */ + private final int maxRetryCount; + + public AbfsRetryPolicy(final int maxRetryCount) { + this.maxRetryCount = maxRetryCount; + } + + /** + * Returns if a request should be retried based on the retry count, current response, + * and the current strategy. The valid http status code lies in the range of 1xx-5xx. + * But an invalid status code might be set due to network or timeout kind of issues. + * Such invalid status code also qualify for retry. + * + * @param retryCount The current retry attempt count. + * @param statusCode The status code of the response, or -1 for socket error. + * @return true if the request should be retried; false otherwise. + */ + public boolean shouldRetry(final int retryCount, final int statusCode) { + return retryCount < this.maxRetryCount Review Comment: no need for "this." ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRestOperation.java: ########## @@ -224,15 +226,18 @@ private void completeExecute(TracingContext tracingContext) requestHeaders.add(httpHeader); } + // By Default Exponential Retry Policy Will be used retryCount = 0; + retryPolicy = client.getExponentialRetryPolicy(); LOG.debug("First execution of REST operation - {}", operationType); while (!executeHttpOperation(retryCount, tracingContext)) { try { ++retryCount; tracingContext.setRetryCount(retryCount); - LOG.debug("Retrying REST operation {}. RetryCount = {}", - operationType, retryCount); - Thread.sleep(client.getRetryPolicy().getRetryInterval(retryCount)); + long retryInterval = retryPolicy.getRetryInterval(retryCount); + LOG.debug("Rest operation {} failed with failureReason: {}. Retrying with retryCount = {}, retryPolicy: {} and sleepInterval: {}", Review Comment: do you think the URL should be logged here too? asking as in #5948 we have been trying to decide what best to log on problems ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRetryPolicy.java: ########## @@ -0,0 +1,73 @@ +/** + * 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.azurebfs.services; + +import java.net.HttpURLConnection; + +import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.HTTP_CONTINUE; + +/** + * Abstract Class for Retry policy to be used by {@link AbfsClient} + * Implementation to be used is based on retry cause. + */ +public abstract class AbfsRetryPolicy { + + /** + * The maximum number of retry attempts. + */ + private final int maxRetryCount; + + public AbfsRetryPolicy(final int maxRetryCount) { Review Comment: 1. make protected; this is abstract and cannot be instantiated. 2. make the abbreviation another final field; the subclasses must provide it in their super() call. ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/StaticRetryPolicy.java: ########## @@ -0,0 +1,68 @@ +/** + * 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.azurebfs.services; + +import org.apache.hadoop.fs.azurebfs.AbfsConfiguration; + +/** + * Retry policy used by AbfsClient for Network Errors. + * */ +public class StaticRetryPolicy extends AbfsRetryPolicy { + + private static final int STATIC_RETRY_INTERVAL_DEFAULT = 2000; // 2s + + /** + * Represents the constant retry interval to be used with Static Retry Policy + */ + private int retryInterval; Review Comment: make final ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRetryPolicy.java: ########## @@ -0,0 +1,73 @@ +/** + * 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.azurebfs.services; + +import java.net.HttpURLConnection; + +import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.HTTP_CONTINUE; + +/** + * Abstract Class for Retry policy to be used by {@link AbfsClient} + * Implementation to be used is based on retry cause. + */ +public abstract class AbfsRetryPolicy { Review Comment: can you add a toString with abbreviation and maxRetryCount; handy for debug and logging ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRestOperation.java: ########## @@ -322,34 +329,47 @@ private boolean executeHttpOperation(final int retryCount, } failureReason = RetryReason.getAbbreviation(ex, -1, ""); - - if (!client.getRetryPolicy().shouldRetry(retryCount, -1)) { + retryPolicy = client.getRetryPolicy(failureReason); + wasIOExceptionThrown = true; + if (!retryPolicy.shouldRetry(retryCount, -1)) { throw new InvalidAbfsRestOperationException(ex, retryCount); } return false; } finally { int status = httpOperation.getStatusCode(); /* - A status less than 300 (2xx range) or greater than or equal - to 500 (5xx range) should contribute to throttling metrics being updated. - Less than 200 or greater than or equal to 500 show failed operations. 2xx - range contributes to successful operations. 3xx range is for redirects - and 4xx range is for user errors. These should not be a part of - throttling backoff computation. - */ + * A status less than 300 (2xx range) or greater than or equal Review Comment: no need for * on every line outside javadocs ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/RetryPolicyConstants.java: ########## @@ -0,0 +1,28 @@ +/** + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.azurebfs.services; + +public final class RetryPolicyConstants { + + private RetryPolicyConstants() { + + } + public static final String EXPONENTIAL_RETRY_POLICY_ABBREVIATION= "E"; Review Comment: nit: javadocs with {@value) ########## hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestTracingContext.java: ########## @@ -226,7 +229,7 @@ fileSystemId, FSOperationType.CREATE_FILESYSTEM, tracingHeaderFormat, new Tracin fs.getFileSystemId(), FSOperationType.CREATE_FILESYSTEM, false, 1)); - tracingContext.constructHeader(abfsHttpOperation, "RT"); + tracingContext.constructHeader(abfsHttpOperation, "RT", "E"); Review Comment: still expecting a ref to the RetryConstants value ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/StaticRetryPolicy.java: ########## @@ -0,0 +1,68 @@ +/** + * 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.azurebfs.services; + +import org.apache.hadoop.fs.azurebfs.AbfsConfiguration; + +/** + * Retry policy used by AbfsClient for Network Errors. + * */ +public class StaticRetryPolicy extends AbfsRetryPolicy { + + private static final int STATIC_RETRY_INTERVAL_DEFAULT = 2000; // 2s Review Comment: can you use 2_000 here? ########## hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/ITestStaticRetryPolicy.java: ########## @@ -0,0 +1,124 @@ +/** + * 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.azurebfs.services; + +import org.assertj.core.api.Assertions; +import org.junit.Test; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.azurebfs.AbstractAbfsIntegrationTest; +import org.apache.hadoop.fs.azurebfs.AzureBlobFileSystem; + +import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.AZURE_MAX_IO_RETRIES; +import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.AZURE_STATIC_RETRY_FOR_CONNECTION_TIMEOUT_ENABLED; +import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.AZURE_STATIC_RETRY_INTERVAL; +import static org.apache.hadoop.fs.azurebfs.services.RetryReasonConstants.CONNECTION_RESET_ABBREVIATION; +import static org.apache.hadoop.fs.azurebfs.services.RetryReasonConstants.CONNECTION_TIMEOUT_ABBREVIATION; + +/** + * Class to test the behavior of Static Retry policy as well the inheritance + * between {@link AbfsRetryPolicy}, {@link ExponentialRetryPolicy}, {@link StaticRetryPolicy} + */ +public class ITestStaticRetryPolicy extends AbstractAbfsIntegrationTest { + + public ITestStaticRetryPolicy() throws Exception { + super(); + } + + /** + * Tests for retry policy related configurations. + * Asserting that the correct retry policy is used for a given set of + * configurations including default ones + * @throws Exception + */ + @Test + public void testStaticRetryPolicyInitializationDefault() throws Exception { + Configuration config = new Configuration(this.getRawConfiguration()); + assertInitialization(config, StaticRetryPolicy.class); + } + + @Test + public void testStaticRetryPolicyInitialization1() throws Exception { + Configuration config = new Configuration(this.getRawConfiguration()); + config.set(AZURE_STATIC_RETRY_FOR_CONNECTION_TIMEOUT_ENABLED, "true"); + assertInitialization(config, StaticRetryPolicy.class); + } + + @Test + public void testStaticRetryPolicyInitialization2() throws Exception { + Configuration config = new Configuration(this.getRawConfiguration()); + config.set(AZURE_STATIC_RETRY_FOR_CONNECTION_TIMEOUT_ENABLED, "false"); + assertInitialization(config, ExponentialRetryPolicy.class); + } + + private void assertInitialization(Configuration config, Class retryPolicyClass) throws Exception{ + final AzureBlobFileSystem fs = (AzureBlobFileSystem) FileSystem + .newInstance(getFileSystem().getUri(), config); + AbfsClient client = fs.getAbfsStore().getClient(); + + // Assert that static retry policy will be used only for CT Failures + AbfsRetryPolicy retryPolicy = client.getRetryPolicy(CONNECTION_TIMEOUT_ABBREVIATION); + Assertions.assertThat(retryPolicy).isInstanceOf(retryPolicyClass); + + // For all other possible values of failureReason, Exponential retry is used + retryPolicy = client.getRetryPolicy(""); + Assertions.assertThat(retryPolicy).isInstanceOf(ExponentialRetryPolicy.class); + retryPolicy = client.getRetryPolicy(null); Review Comment: this is repeated enough that it should be factored out ``` assertIsExponentialPolicy(client, policy) ``` -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
