[ https://issues.apache.org/jira/browse/HADOOP-18606?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17683117#comment-17683117 ]
ASF GitHub Bot commented on HADOOP-18606: ----------------------------------------- steveloughran commented on code in PR #5299: URL: https://github.com/apache/hadoop/pull/5299#discussion_r1093487300 ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/AbfsHttpConstants.java: ########## @@ -111,6 +111,15 @@ public final class AbfsHttpConstants { public static final char CHAR_EQUALS = '='; public static final char CHAR_STAR = '*'; public static final char CHAR_PLUS = '+'; + /** Review Comment: javadoc nits. 1. add a line break. 2. for those lines you want well laid out, just use a \<pre> above and a </pre> below, remove those \<br> tags ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRestOperation.java: ########## @@ -237,6 +240,11 @@ private void completeExecute(TracingContext tracingContext) LOG.trace("{} REST operation complete", operationType); } + @VisibleForTesting + String getClientLatency() { + return this.client.getAbfsPerfTracker().getClientLatency(); Review Comment: remove the `this.`. I know, it was there before, but this is time to clean up. ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRestOperation.java: ########## @@ -73,6 +74,8 @@ public class AbfsRestOperation { private AbfsHttpOperation result; private AbfsCounters abfsCounters; + private String failureReason = null; Review Comment: add a javadoc to explain. no need to set as null as that is the default and it actually speeds up object creation to not set it ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRestOperation.java: ########## @@ -334,6 +347,11 @@ private boolean executeHttpOperation(final int retryCount, return true; } + @VisibleForTesting + AbfsHttpOperation getHttpOperation() throws IOException { Review Comment: as this is more than just a getter, can you * call it `createHttpOperation()` * add a javadoc description ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/RetryReason.java: ########## @@ -0,0 +1,178 @@ +/** + * 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; + +import java.io.IOException; +import java.net.SocketException; +import java.net.UnknownHostException; +import java.util.ArrayList; +import java.util.List; + +import static java.net.HttpURLConnection.HTTP_UNAVAILABLE; +import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.HTTP_STATUS_CATEGORY_QUOTIENT; + +/** + * In case of retry, this enum would give the information on the reason for + * previous API call. + * */ +public enum RetryReason { + CONNECTION_TIMEOUT(2, + ((exceptionCaptured, statusCode, serverErrorMessage) -> { + if (exceptionCaptured != null && "connect timed out".equalsIgnoreCase( Review Comment: can you do a line break before the && ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/RetryReason.java: ########## @@ -0,0 +1,178 @@ +/** + * 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; + +import java.io.IOException; +import java.net.SocketException; +import java.net.UnknownHostException; +import java.util.ArrayList; +import java.util.List; + +import static java.net.HttpURLConnection.HTTP_UNAVAILABLE; +import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.HTTP_STATUS_CATEGORY_QUOTIENT; + +/** + * In case of retry, this enum would give the information on the reason for + * previous API call. + * */ +public enum RetryReason { + CONNECTION_TIMEOUT(2, Review Comment: add a javadoc above every enum ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/RetryReason.java: ########## @@ -0,0 +1,178 @@ +/** + * 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; + +import java.io.IOException; +import java.net.SocketException; +import java.net.UnknownHostException; +import java.util.ArrayList; +import java.util.List; + +import static java.net.HttpURLConnection.HTTP_UNAVAILABLE; +import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.HTTP_STATUS_CATEGORY_QUOTIENT; + +/** + * In case of retry, this enum would give the information on the reason for + * previous API call. + * */ +public enum RetryReason { + CONNECTION_TIMEOUT(2, + ((exceptionCaptured, statusCode, serverErrorMessage) -> { + if (exceptionCaptured != null && "connect timed out".equalsIgnoreCase( + exceptionCaptured.getMessage())) { + return "CT"; + } + return null; + })), + READ_TIMEOUT(2, ((exceptionCaptured, statusCode, serverErrorMessage) -> { + if (exceptionCaptured != null && "Read timed out".equalsIgnoreCase( + exceptionCaptured.getMessage())) { + return "RT"; + } + return null; + })), + UNKNOWN_HOST(2, ((exceptionCaptured, statusCode, serverErrorMessage) -> { + if (exceptionCaptured instanceof UnknownHostException) { + return "UH"; + } + return null; + })), + CONNECTION_RESET(2, ((exceptionCaptured, statusCode, serverErrorMessage) -> { + if (exceptionCaptured != null && exceptionCaptured.getMessage() != null + && exceptionCaptured.getMessage().contains("Connection reset")) { Review Comment: convert toLower(EN_US) and have a lower case string to look up. ########## hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsRestOperation.java: ########## @@ -0,0 +1,287 @@ +/** + * 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; + +import java.io.IOException; +import java.io.InterruptedIOException; +import java.net.HttpURLConnection; +import java.net.SocketException; +import java.net.SocketTimeoutException; +import java.net.UnknownHostException; +import java.util.ArrayList; + +import org.assertj.core.api.Assertions; +import org.junit.Test; +import org.mockito.Mockito; +import org.mockito.stubbing.Stubber; + +import org.apache.hadoop.fs.azurebfs.utils.TracingContext; + +import static java.net.HttpURLConnection.HTTP_BAD_REQUEST; +import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR; +import static java.net.HttpURLConnection.HTTP_OK; +import static java.net.HttpURLConnection.HTTP_UNAVAILABLE; +import static org.apache.hadoop.fs.azurebfs.services.AuthType.OAuth; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.nullable; + +public class TestAbfsRestOperation { Review Comment: name of test should match the feature better, eg TestAbfsRestOperationMockFailures > Add reason in in x-ms-client-request-id on a retry API call. > ------------------------------------------------------------ > > Key: HADOOP-18606 > URL: https://issues.apache.org/jira/browse/HADOOP-18606 > Project: Hadoop Common > Issue Type: Sub-task > Components: fs/azure > Reporter: Pranav Saxena > Assignee: Pranav Saxena > Priority: Minor > Labels: pull-request-available > Fix For: 3.4.0 > > > In the header, x-ms-client-request-id contains informaiton on what retry this > particular API call is: for ex: > :eb06d8f6-5693-461b-b63c-5858fa7655e6:29cb0d19-2b68-4409-bc35-cb7160b90dd8:::CF:1. > We want to add the reason for the retry in the header_value:Now the same > header would include retry reason in case its not the 0th iteration of the > API operation. It would be like > :eb06d8f6-5693-461b-b63c-5858fa7655e6:29cb0d19-2b68-4409-bc35-cb7160b90dd8:::CF:1_RT. > This corresponds that its retry number 1. The 0th iteration was failed due > to read timeout. -- This message was sent by Atlassian Jira (v8.20.10#820010) --------------------------------------------------------------------- To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org