HADOOP-19096: [ABFS] [CST Optimization] Enhancing Client-Side Throttling Metrics Updating Logic #6276
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
added 2 commits
November 15, 2023 22:19
...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRestOperation.java
Outdated
Show resolved
Hide resolved
...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRestOperation.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/hadoop/fs/azurebfs/services/retryReasonCategories/ServerErrorRetryReason.java
Outdated
Show resolved
Hide resolved
.../src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsRestOperationMockFailures.java
Outdated
Show resolved
Hide resolved
| import static org.mockito.ArgumentMatchers.any; | ||
| import static org.mockito.ArgumentMatchers.nullable; | ||
|
|
||
| public class TestAbfsRestOperationMockFailures { | ||
| // In these tests a request first fails with given exceptions and then succeed on retry. |
There was a problem hiding this comment.
Add a test where number of time metrics are updated for say 500 kind of error code is 0
There was a problem hiding this comment.
testClientRequestIdFor500Retry asserts this.
Here the call first fails with 500 and then succeed. CST metrics are updated only once after success.
...tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestRetryReason.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRestOperation.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
added 2 commits
February 28, 2024 21:11
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
anujmodi2021
changed the title
saxenapranav
approved these changes
Mar 1, 2024
|
🎊 +1 overall
This message was automatically generated. |
|
Hi @steveloughran , @mukund-thakur Thanks |
rakeshadr
reviewed
Mar 26, 2024
| @@ -56,10 +58,14 @@ String getAbbreviation(final Integer statusCode, | |||
| splitedServerErrorMessage)) { | |||
| return EGRESS_LIMIT_BREACH_ABBREVIATION; | |||
| } | |||
| if (OPERATION_BREACH_MESSAGE.equalsIgnoreCase( | |||
| if (TPS_OVER_ACCOUNT_LIMIT.getErrorMessage().equalsIgnoreCase( | |||
| splitedServerErrorMessage)) { | |||
| return OPERATION_LIMIT_BREACH_ABBREVIATION; | |||
There was a problem hiding this comment.
Could you please rename OPERATION_LIMIT_BREACH_ABBREVIATION to TPS_LIMIT_BREACH_ABBREVIATION, this would maintain consistency with the naming convention with other constants.
| || INGRESS_LIMIT_BREACH_ABBREVIATION.equals(failureReason) // Case 4.a | ||
| || EGRESS_LIMIT_BREACH_ABBREVIATION.equals(failureReason) // Case 4.b | ||
| || OPERATION_LIMIT_BREACH_ABBREVIATION.equals(failureReason)) // Case 4.c | ||
| && !wasExceptionThrown; // Case 6 |
There was a problem hiding this comment.
Can you please encapsulate the chain of conditions to a method #shouldUpdateCSTMetrics()
| // If no exception occurred till here it means http operation was successfully complete and | ||
| // a response from server has been received which might be failure or success. | ||
| // If any kind of exception has occurred it will be caught below. | ||
| // If request failed determine failure reason and retry policy here. |
| } catch (UnknownHostException ex) { | ||
| wasExceptionThrown = true; |
There was a problem hiding this comment.
should we rename this? because in case of other exceptions, we update the metric below in finally block.
There was a problem hiding this comment.
Changed it to wasKnownExceptionThrown.
So that if any other exception is thrown we will still update metrics
| @@ -283,7 +285,7 @@ String getClientLatency() { | |||
| private boolean executeHttpOperation(final int retryCount, | |||
| TracingContext tracingContext) throws AzureBlobFileSystemException { | |||
| AbfsHttpOperation httpOperation; | |||
| boolean wasIOExceptionThrown = false; | |||
| boolean wasExceptionThrown = false; | |||
There was a problem hiding this comment.
maybe add a comment on the usage of this,
There was a problem hiding this comment.
Added comment
Thanks for the review. |
|
🎊 +1 overall
This message was automatically generated. |
|
thanks, in trunk. @anmolanmol1234 can you do a pr and retest for 3.4? I'll rebase my work on this and see how it goes |
That would be me, not anmol. |
anujmodi2021
added a commit
to anujmodi2021/hadoop
that referenced
this pull request
Apr 11, 2024
…g Metrics Logic (apache#6276) ABFS has a client-side throttling mechanism which works on the metrics collected from past requests When requests are fail due to server-side throttling it updates its metrics and recalculates any client side backoff. The choice of which requests should be used to compute client side backoff interval is based on the http status code: - Status code in 2xx range: Successful Operations should contribute. - Status code in 3xx range: Redirection Operations should not contribute. - Status code in 4xx range: User Errors should not contribute. - Status code is 503: Throttling Error should contribute only if they are due to client limits breach as follows: * 503, Ingress Over Account Limit: Should Contribute * 503, Egress Over Account Limit: Should Contribute * 503, TPS Over Account Limit: Should Contribute * 503, Other Server Throttling: Should not Contribute. - Status code in 5xx range other than 503: Should not Contribute. - IOException and UnknownHostExceptions: Should not Contribute. Contributed by Anuj Modi
steveloughran
pushed a commit
that referenced
this pull request
Apr 11, 2024
…g Metrics Logic (#6276) ABFS has a client-side throttling mechanism which works on the metrics collected from past requests When requests are fail due to server-side throttling it updates its metrics and recalculates any client side backoff. The choice of which requests should be used to compute client side backoff interval is based on the http status code: - Status code in 2xx range: Successful Operations should contribute. - Status code in 3xx range: Redirection Operations should not contribute. - Status code in 4xx range: User Errors should not contribute. - Status code is 503: Throttling Error should contribute only if they are due to client limits breach as follows: * 503, Ingress Over Account Limit: Should Contribute * 503, Egress Over Account Limit: Should Contribute * 503, TPS Over Account Limit: Should Contribute * 503, Other Server Throttling: Should not Contribute. - Status code in 5xx range other than 503: Should not Contribute. - IOException and UnknownHostExceptions: Should not Contribute. Contributed by Anuj Modi
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description of PR
Jira: https://issues.apache.org/jira/browse/HADOOP-19096
ABFS has a client-side throttling mechanism which works on the metrics collected from past requests made. I requests are getting failed due to throttling at server, we update our metrics and client side backoff is calculated based on those metrics.
This PR enhances the logic to decide which requests should be considered to compute client side backoff interval as follows:
For each request made by ABFS driver, we will determine if they should contribute to Client-Side Throttling based on the status code and result:
How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?