HADOOP-17092. ABFS: Making AzureADAuthenticator.getToken() throw HttpException if a… #2123
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
…ll the retries are failed. This is to indiacate no retry is required at AbfsRestOperation.executeHttpOperation(). Introduced delay inbetween retries.
|
Javadoc is failing in the trunk as well. Please find the JIRA HADOOP-17085 Driver test results using accounts in Central India Account with HNS Support Account without HNS support |
|
Driver test results using accounts in Central India Account with HNS Support Account without HNS support |
|
💔 -1 overall
This message was automatically generated. |
snvijaya
suggested changes
Jul 13, 2020
| * @return {@link AzureADToken} obtained using the creds | ||
| * @throws IOException throws IOException if there is a failure in connecting to Azure AD | ||
| */ | ||
| public static AzureADToken getTokenUsingClientCreds(String authEndpoint, | ||
| String clientId, String clientSecret) | ||
| String clientId, | ||
| String clientSecret, ExponentialRetryPolicy tokenFetchRetryPolicy) |
|
|
||
| int httperror = 0; | ||
| IOException ex = null; | ||
| boolean succeeded = false; | ||
| int retryCount = 0; | ||
| boolean shouldRetry; | ||
| LOG.debug("First execution of REST operation getTokenSingleCall"); |
There was a problem hiding this comment.
Make this a trace level log.
| throws IOException { | ||
| AzureADToken token = null; | ||
| ExponentialRetryPolicy retryPolicy |
There was a problem hiding this comment.
As every OAuth token provider eventually calls this method to get token, you will just need to create an instance of exponential retry right here with the configured values ?
Is there any benefit creating and passing down exponential retry instance from each of the token provider ?
|
Maybe track in different JIRA, but test/fixes to ensure token fetch code is identifying recoverable (i.e 'should retry') and non-recoverable scenarios correctly. non-recoverable one shouldn't be retried. |
bilaharith
changed the title
|
💔 -1 overall
This message was automatically generated. |
|
Driver test results using accounts in Central India Account with HNS Support Account without HNS support |
|
Driver test results using accounts in Central India Account with HNS Support Account without HNS support |
snvijaya
approved these changes
Jul 16, 2020
|
💔 -1 overall
This message was automatically generated. |
DadanielZ
requested changes
Jul 16, 2020
| @@ -50,6 +50,7 @@ | |||
| import com.google.common.annotations.VisibleForTesting; | |||
| import com.google.common.base.Preconditions; | |||
| import com.google.common.base.Strings; | |||
| import org.apache.hadoop.fs.azurebfs.oauth2.AzureADAuthenticator; | |||
|
💔 -1 overall
This message was automatically generated. |
Fixing import order
bilaharith
force-pushed
the
HADOOP-17092-gettokenretry
branch
from
50b203c to
2b1886b
Compare
|
💔 -1 overall
This message was automatically generated. |
|
not going to suggest any changes of my own. One thing to know in future is that one of the hadoop retry policies takes a map of other policies and dynamically chooses the correct one based on the exception raised. Not needed here with only two exceptions treated as recoverable, but if you want to do more complex things, especially handle throttling, worth looking at see org.apache.hadoop.fs.s3a.S3ARetryPolicy for it in action |
|
💔 -1 overall
This message was automatically generated. |
Sure. |
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.
ABFS: Making AzureADAuthenticator.getToken() throw HttpException if all the retries are failed. This is to indiacate no retry is required at AbfsRestOperation.executeHttpOperation(). Introduced delay inbetween retries.
Test cases are not added as the change is in a private static method.
Driver test results using accounts in Central India
mvn -T 1C -Dparallel-tests=abfs -Dscale -DtestsThreadCount=8 clean verify
Account with HNS Support
[INFO] Tests run: 63, Failures: 0, Errors: 0, Skipped: 0
[WARNING] Tests run: 436, Failures: 0, Errors: 0, Skipped: 74
[WARNING] Tests run: 206, Failures: 0, Errors: 0, Skipped: 24
Account without HNS support
[INFO] Tests run: 63, Failures: 0, Errors: 0, Skipped: 0
[WARNING] Tests run: 436, Failures: 0, Errors: 0, Skipped: 248
[WARNING] Tests run: 206, Failures: 0, Errors: 0, Skipped: 24