HADOOP-18606. Add reason in in x-ms-client-request-id on a retry API call. #5299
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/RetryReason.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/RetryReason.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/utils/TracingContext.java
Outdated
Show resolved
Hide resolved
...hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsRestOperation.java
Outdated
Show resolved
Hide resolved
| import static org.mockito.ArgumentMatchers.nullable; | ||
|
|
||
| public class TestAbfsRestOperation { | ||
|
|
Contributor
There was a problem hiding this comment.
Can be implemented using parameterized functionality because in every function the only the failure reason is getting modified.
Contributor
Author
There was a problem hiding this comment.
Have made method for common logics. The tests would send only parameters into the common test-logic method.
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/utils/TracingContext.java
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
|
💔 -1 overall
This message was automatically generated. |
This comment was marked as outdated.
This comment was marked as outdated.
|
💔 -1 overall
This message was automatically generated. |
This comment was marked as outdated.
This comment was marked as outdated.
|
💔 -1 overall
This message was automatically generated. |
This comment was marked as outdated.
This comment was marked as outdated.
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/RetryReason.java
Outdated
Show resolved
Hide resolved
| Mockito.doReturn(httpURLConnection).when(httpOperation).getConnection(); | ||
| Mockito.doReturn("").when(abfsRestOperation).getClientLatency(); | ||
|
|
||
| //new AbfsHttpOperation(null, "PUT", new ArrayList<>())); |
Contributor
There was a problem hiding this comment.
Remove unwanted comments.
Review-comments for pullRequest 5299
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
anmolanmol1234
approved these changes
Feb 28, 2023
Contributor
Author
|
Hi @steveloughran,
Thanks. |
|
💔 -1 overall
This message was automatically generated. |
steveloughran
requested changes
Mar 3, 2023
Contributor
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
I'm happy with all the code!
just a little bit of javadoc tuning and we are done!
+1 pending the changes
...src/main/java/org/apache/hadoop/fs/azurebfs/services/retryReasonCategories/package-info.java
Show resolved
Hide resolved
| package org.apache.hadoop.fs.azurebfs.services.retryReasonCategories; | ||
|
|
||
| import org.apache.hadoop.classification.InterfaceAudience.Private; | ||
| import org.apache.hadoop.classification.InterfaceStability.Evolving; No newline at end of file |
Contributor
There was a problem hiding this comment.
nit: please add a newline
Contributor
Author
There was a problem hiding this comment.
Added new line.
| /** | ||
| * This utility class exposes methods to convert a server response-error to a | ||
| * category of error. | ||
| * */ |
Contributor
There was a problem hiding this comment.
nit, can you replace the lines in this package which have
* */
with
*/
I know IDEs love to do things like this...it is just now is the time to get them right as nobody will justify doing it later
Contributor
Author
There was a problem hiding this comment.
Have fixed the javadoc ending to */ in all the changed code-pieces.
Contributor
Author
Thank you so much @steveloughran. Have made changes as per the review comments. Thanks. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
|
ok, merged into trunk. can you do the cherrypick and retest for branch-3.3, and push that up as a new PR just so it can get through yetus. no more code changes, just backport validation. |
saxenapranav
added a commit
to saxenapranav/hadoop
that referenced
this pull request
Mar 8, 2023
steveloughran
pushed a commit
that referenced
this pull request
Mar 28, 2023
… API call. (#5299) Contributed by Pranav Saxena
ferdelyi
pushed a commit
to ferdelyi/hadoop
that referenced
this pull request
May 26, 2023
… API call. (apache#5299) Contributed by Pranav Saxena
saxenapranav
added a commit
to saxenapranav/hadoop
that referenced
this pull request
Jun 1, 2023
… API call. (apache#5299) Contributed by Pranav Saxena (cherry picked from commit 358bf80)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
JIRA: https://issues.apache.org/jira/browse/HADOOP-18606
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.
Example when readTimeout is happening two retries for same operation.
:c4220e3a-8498-4217-88ff-403e2a6864fc:29cb0d19-2b68-4409-bc35-cb7160b90dd8:::CF:0
:eb06d8f6-5693-461b-b63c-5858fa7655e6:29cb0d19-2b68-4409-bc35-cb7160b90dd8:::CF:1_RT
:c4b3b2b4-0abc-44fd-86f9-885378b9895c:29cb0d19-2b68-4409-bc35-cb7160b90dd8:::CF:2_RT
:::: AGGREGATED TEST RESULT ::::
HNS-OAuth
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] TestAccountConfiguration.testConfigPropNotFound:386->testMissingConfigKey:399 Expected a org.apache.hadoop.fs.azurebfs.contracts.exceptions.TokenAccessProviderException to be thrown, but got the result: : "org.apache.hadoop.fs.azurebfs.oauth2.ClientCredsTokenProvider"
[ERROR] Errors:
[ERROR] TestExponentialRetryPolicy.testOperationOnAccountIdle:216 » AccessDenied Opera...
[INFO]
[ERROR] Tests run: 124, Failures: 1, Errors: 1, Skipped: 1
[INFO] Results:
[INFO]
[ERROR] Errors:
[ERROR] ITestAzureBlobFileSystemLease.testAcquireRetry:329 » TestTimedOut test timed o...
[ERROR] ITestAzureBlobFileSystemOauth.testBlobDataContributor:84 » AccessDenied Operat...
[ERROR] ITestAzureBlobFileSystemOauth.testBlobDataReader:143 » AccessDenied Operation ...
[INFO]
[ERROR] Tests run: 568, Failures: 0, Errors: 3, Skipped: 99
[INFO] Results:
[INFO]
[ERROR] Errors:
[ERROR] ITestAbfsTerasort.test_120_terasort:262->executeStage:206 » IO The ownership o...
[INFO]
[ERROR] Tests run: 335, Failures: 0, Errors: 1, Skipped: 54
HNS-SharedKey
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] TestAccountConfiguration.testConfigPropNotFound:386->testMissingConfigKey:399 Expected a org.apache.hadoop.fs.azurebfs.contracts.exceptions.TokenAccessProviderException to be thrown, but got the result: : "org.apache.hadoop.fs.azurebfs.oauth2.ClientCredsTokenProvider"
[ERROR] Errors:
[ERROR] TestExponentialRetryPolicy.testOperationOnAccountIdle:216 » AccessDenied Opera...
[INFO]
[ERROR] Tests run: 124, Failures: 1, Errors: 1, Skipped: 2
[INFO] Results:
[INFO]
[ERROR] Errors:
[ERROR] ITestAzureBlobFileSystemLease.testAcquireRetry:329 » TestTimedOut test timed o...
[INFO]
[ERROR] Tests run: 568, Failures: 0, Errors: 1, Skipped: 54
[INFO] Results:
[INFO]
[WARNING] Tests run: 335, Failures: 0, Errors: 0, Skipped: 41
NonHNS-SharedKey
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] TestAccountConfiguration.testConfigPropNotFound:386->testMissingConfigKey:399 Expected a org.apache.hadoop.fs.azurebfs.contracts.exceptions.TokenAccessProviderException to be thrown, but got the result: : "org.apache.hadoop.fs.azurebfs.oauth2.ClientCredsTokenProvider"
[ERROR] Errors:
[ERROR] TestExponentialRetryPolicy.testOperationOnAccountIdle:216 » AccessDenied Opera...
[INFO]
[ERROR] Tests run: 124, Failures: 1, Errors: 1, Skipped: 2
[INFO] Results:
[INFO]
[ERROR] Errors:
[ERROR] ITestAzureBlobFileSystemLease.testAcquireRetry:336 » TestTimedOut test timed o...
[INFO]
[ERROR] Tests run: 568, Failures: 0, Errors: 1, Skipped: 277
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] ITestAbfsTerasort.test_110_teragen:244->executeStage:211->Assert.assertEquals:647->Assert.failNotEquals:835->Assert.fail:89 teragen(1000, abfs://[email protected]/ITestAbfsTerasort/sortin) failed expected:<0> but was:<1>
[ERROR] Errors:
[ERROR] ITestAbfsJobThroughManifestCommitter.test_0420_validateJob » OutputValidation ...
[ERROR] ITestAbfsManifestCommitProtocol.testCommitLifecycle » OutputValidation
abfs:/... [ERROR] ITestAbfsManifestCommitProtocol.testCommitterWithDuplicatedCommit » OutputValidation [ERROR] ITestAbfsManifestCommitProtocol.testConcurrentCommitTaskWithSubDir » OutputValidation [ERROR] ITestAbfsManifestCommitProtocol.testMapFileOutputCommitter » OutputValidation ... [ERROR] ITestAbfsManifestCommitProtocol.testOutputFormatIntegration » OutputValidation [ERROR] ITestAbfsManifestCommitProtocol.testParallelJobsToAdjacentPaths » OutputValidation [ERROR] ITestAbfsManifestCommitProtocol.testTwoTaskAttemptsCommit » OutputValidation...[INFO]
[ERROR] Tests run: 335, Failures: 1, Errors: 8, Skipped: 46
AppendBlob-HNS-OAuth
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] TestAccountConfiguration.testConfigPropNotFound:386->testMissingConfigKey:399 Expected a org.apache.hadoop.fs.azurebfs.contracts.exceptions.TokenAccessProviderException to be thrown, but got the result: : "org.apache.hadoop.fs.azurebfs.oauth2.ClientCredsTokenProvider"
[ERROR] Errors:
[ERROR] TestExponentialRetryPolicy.testOperationOnAccountIdle:216 » AccessDenied Opera...
[INFO]
[ERROR] Tests run: 124, Failures: 1, Errors: 1, Skipped: 1
[INFO] Results:
[INFO]
[ERROR] Errors:
[ERROR] ITestAzureBlobFileSystemLease.testAcquireRetry:329 » TestTimedOut test timed o...
[ERROR] ITestAzureBlobFileSystemOauth.testBlobDataContributor:84 » AccessDenied Operat...
[ERROR] ITestAzureBlobFileSystemOauth.testBlobDataReader:143 » AccessDenied Operation ...
[INFO]
[ERROR] Tests run: 568, Failures: 0, Errors: 3, Skipped: 99
[INFO] Results:
[INFO]
[ERROR] Errors:
[ERROR] ITestAbfsTerasort.test_120_terasort:262->executeStage:206 » IO The ownership o...
[INFO]
[ERROR] Tests run: 335, Failures: 0, Errors: 1, Skipped: 54
Time taken: 43 mins 18 secs.