steveloughran commented on code in PR #5273:
URL: https://github.com/apache/hadoop/pull/5273#discussion_r1164119014
##########
hadoop-tools/hadoop-azure/pom.xml:
##########
@@ -321,8 +321,23 @@
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
+ <version>4.11.0</version>
Review Comment:
sorry, hadoop-project defines the version, and through properties. revert
this
##########
hadoop-tools/hadoop-azure/pom.xml:
##########
@@ -321,8 +321,23 @@
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
+ <version>4.11.0</version>
<scope>test</scope>
</dependency>
+
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-inline</artifactId>
+ <version>4.11.0</version>
Review Comment:
if this is new to hadoop, declare it in hadoop-project/pom.xml, with
versions and exclusions, then declare here without those
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsMsiTokenProvider.java:
##########
@@ -40,13 +47,16 @@
import static
org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.FS_AZURE_ACCOUNT_OAUTH_MSI_AUTHORITY;
import static
org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.FS_AZURE_ACCOUNT_OAUTH_MSI_ENDPOINT;
import static
org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.FS_AZURE_ACCOUNT_OAUTH_MSI_TENANT;
+import static org.mockito.Mockito.times;
/**
* Test MsiTokenProvider.
*/
public final class ITestAbfsMsiTokenProvider
extends AbstractAbfsIntegrationTest {
+ private static final int HTTP_TOO_MANY_REQUESTS = 429;
Review Comment:
refer to the value in the src/main code
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ExponentialRetryPolicy.java:
##########
@@ -58,6 +58,13 @@ public class ExponentialRetryPolicy {
*/
private static final double MAX_RANDOM_RATIO = 1.2;
+ /**
+ * Qualifies for retry based on
Review Comment:
needs a . in it, maybe split into
"qualifies for retry."
and "see..."
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsMsiTokenProvider.java:
##########
@@ -90,4 +100,109 @@ private String getTrimmedPasswordString(AbfsConfiguration
conf, String key,
return value.trim();
}
+ /**
+ * Test to verify that token fetch is retried for throttling errors (too
many requests 429).
+ * @throws Exception
+ */
+ @Test
+ public void testRetryForThrottling() throws Exception {
+ AbfsConfiguration conf = getConfiguration();
+
+ // Exception to be thrown with throttling error code 429.
+ AzureADAuthenticator.HttpException httpException
+ = new AzureADAuthenticator.HttpException(HTTP_TOO_MANY_REQUESTS,
+ "abc", "abc", "abc", "abc", "abc");
+
+ String tenantGuid = "abcd";
+ String clientId = "abcd";
+ String authEndpoint = getTrimmedPasswordString(conf,
Review Comment:
what if these are undefined? skip the test?
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsMsiTokenProvider.java:
##########
@@ -90,4 +100,109 @@ private String getTrimmedPasswordString(AbfsConfiguration
conf, String key,
return value.trim();
}
+ /**
+ * Test to verify that token fetch is retried for throttling errors (too
many requests 429).
+ * @throws Exception
Review Comment:
cut this @throws
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/oauth2/AzureADAuthenticator.java:
##########
@@ -341,7 +341,7 @@ private static boolean isRecoverableFailure(IOException e) {
|| e instanceof FileNotFoundException);
}
- private static AzureADToken getTokenSingleCall(String authEndpoint,
+ public static AzureADToken getTokenSingleCall(String authEndpoint,
Review Comment:
now it is public, add a javadoc
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ExponentialRetryPolicy.java:
##########
@@ -58,6 +58,13 @@ public class ExponentialRetryPolicy {
*/
private static final double MAX_RANDOM_RATIO = 1.2;
+ /**
+ * Qualifies for retry based on
+ * https://learn.microsoft.com/en-us/azure/active-directory/
+ * managed-identities-azure-resources/how-to-use-vm-token#error-handling
+ */
+ private static final int HTTP_TOO_MANY_REQUESTS = 429;
Review Comment:
make public and refer from tests, maybe put in a different file for this
--
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]
