saxenapranav commented on code in PR #6276:
URL: https://github.com/apache/hadoop/pull/6276#discussion_r1395841543
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRestOperation.java:
##########
@@ -337,32 +356,31 @@ private boolean executeHttpOperation(final int 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.
+ Updating Client Side Throttling Metrics for relevant response status
codes.
+ 1. Status code in 2xx range: Successful Operations should contribute
+ 2. Status code in 3xx range: Redirection Operations should not
contribute
+ 3. Status code in 4xx range: User Errors should not contribute
+ 4. Status code in 503 range: Server Error should contribute as
following:
+ a. 503, Ingress Over Account Limit: Should Contribute
+ b. 503, Egress Over Account Limit: Should Contribute
+ c. 503, TPS Over Account Limit: Should Contribute
+ d. 503, Other Server Throttling: Should not contribute
+ 5. Status code in 5xx range other than 503: Should not contribute
+ 6. IOException and UnknownHostExceptions: Should not contribute
*/
- boolean updateMetricsResponseCode = (status <
HttpURLConnection.HTTP_MULT_CHOICE
- || status >= HttpURLConnection.HTTP_INTERNAL_ERROR);
- if (updateMetricsResponseCode) {
+ int statusCode = httpOperation.getStatusCode();
+ boolean shouldUpdateCSTMetrics = (statusCode <
HttpURLConnection.HTTP_MULT_CHOICE // Case 1
Review Comment:
should we do
`(statusCode < MULT_CHOICE || (status > INTERNAL_ERROR &&
!OTHER_SERVER_THROTTLING_ABBREVIATION.equals(failureReason))) &&
!wasExceptionThrown`
reason being: in `AbfsClientThrottlingIntercept`, `isFailedOperation`
depends on `status >= INTERNAL_ERROR`.
+in case its the last possible retry of retryLoop, in that case,
failureReason will not get updated.
If we use above heuristic, we can remove dependency on failureReason for
checking condition. in that case we would have to do:
```
(statusCode < MULT_CHOICE || (status > INTERNAL_ERROR &&
!OTHER_SERVER_THROTTLING.getErrorMessage().equals(httpOperation.getStorageErrorMessage())))
&& !wasExceptionThrown
```
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRestOperation.java:
##########
@@ -337,32 +356,31 @@ private boolean executeHttpOperation(final int 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.
+ Updating Client Side Throttling Metrics for relevant response status
codes.
+ 1. Status code in 2xx range: Successful Operations should contribute
+ 2. Status code in 3xx range: Redirection Operations should not
contribute
+ 3. Status code in 4xx range: User Errors should not contribute
+ 4. Status code in 503 range: Server Error should contribute as
following:
+ a. 503, Ingress Over Account Limit: Should Contribute
+ b. 503, Egress Over Account Limit: Should Contribute
+ c. 503, TPS Over Account Limit: Should Contribute
+ d. 503, Other Server Throttling: Should not contribute
+ 5. Status code in 5xx range other than 503: Should not contribute
+ 6. IOException and UnknownHostExceptions: Should not contribute
*/
- boolean updateMetricsResponseCode = (status <
HttpURLConnection.HTTP_MULT_CHOICE
- || status >= HttpURLConnection.HTTP_INTERNAL_ERROR);
- if (updateMetricsResponseCode) {
+ int statusCode = httpOperation.getStatusCode();
+ boolean shouldUpdateCSTMetrics = (statusCode <
HttpURLConnection.HTTP_MULT_CHOICE // Case 1
Review Comment:
Also, was thinking about `wasExceptionThown`. Because apart from read
timeout, JDK would be setting -1 as status for all exception (-1 is default).
Was Looking in SocketInputStream of java.net, setting -1 is not there.
In case of readTimeout happens, setting of -1 will not happen if we are
trying read big block and there is read timeout in reading next segment. If
first segment is timeouted, status shall be -1 (statusCode is read from the
inputstream created on connection.).
So, if we know that in exception, status shall be -1, we can think of
removing this condition. Because, in the case of segment timeout, server still
would have returned with non-503 status(thats why we would have started reading
it in abfsHttpOp#processResponse (status <400)). We can discuss if we want to
take segment-timeout status or not.
But, its good to have extra prevention.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
