steveloughran commented on code in PR #5024:
URL: https://github.com/apache/hadoop/pull/5024#discussion_r1004768008
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/auth/AbstractSessionCredentialsProvider.java:
##########
@@ -132,13 +134,15 @@ public AWSCredentials getCredentials() throws
SdkBaseException {
}
if (awsCredentials == null) {
throw new CredentialInitializationException(
- "Provider " + this + " has no credentials");
+ "Provider " + this + " has no credentials: " +
+ (initializationException!=null ?
initializationException.getMessage() : ""),
Review Comment:
make it initializationException.toString(), so things like
NullPointerException get mentions. also, put spaces around the !=
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AAWSCredentialsProvider.java:
##########
@@ -480,4 +488,149 @@ public void refresh() {
}
}
+ private static AWSCredentials expectedCredentials = new AWSCredentials() {
+ @Override
+ public String getAWSAccessKeyId() {
+ return "expectedAccessKey";
+ }
+
+ @Override
+ public String getAWSSecretKey() {
+ return "expectedSecret";
+ }
+ };
+
+ /**
+ * Credential provider that takes a long time.
+ */
+ private static class SlowProvider extends AbstractSessionCredentialsProvider
{
+
+ public SlowProvider(@Nullable URI uri, Configuration conf) {
+ super(uri, conf);
+ }
+
+ @Override
+ protected AWSCredentials createCredentials(Configuration config) throws
IOException {
+ // yield to other callers to induce race condition
+ Thread.yield();
+ return expectedCredentials;
+ }
+ }
+
+ @Test
+ public void testConcurrentAuthentication() throws Throwable {
+ Configuration conf =
createProviderConfiguration(SlowProvider.class.getName());
+ Path testFile = getCSVTestPath(conf);
+
+ int threads = 10;
+
+ AWSCredentialProviderList list =
createAWSCredentialProviderSet(testFile.toUri(), conf);
+
+ SlowProvider provider = (SlowProvider) list.getProviders().get(0);
+
+ ExecutorService pool = Executors.newFixedThreadPool(threads);
Review Comment:
* the #of threads should be a constant, shared across the new tests.
* pool should be shutdown in a finally{} clause in both test casees
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AAWSCredentialsProvider.java:
##########
@@ -480,4 +488,149 @@ public void refresh() {
}
}
+ private static AWSCredentials expectedCredentials = new AWSCredentials() {
+ @Override
+ public String getAWSAccessKeyId() {
+ return "expectedAccessKey";
+ }
+
+ @Override
+ public String getAWSSecretKey() {
+ return "expectedSecret";
+ }
+ };
+
+ /**
+ * Credential provider that takes a long time.
+ */
+ private static class SlowProvider extends AbstractSessionCredentialsProvider
{
+
+ public SlowProvider(@Nullable URI uri, Configuration conf) {
+ super(uri, conf);
+ }
+
+ @Override
+ protected AWSCredentials createCredentials(Configuration config) throws
IOException {
+ // yield to other callers to induce race condition
+ Thread.yield();
+ return expectedCredentials;
+ }
+ }
+
+ @Test
+ public void testConcurrentAuthentication() throws Throwable {
+ Configuration conf =
createProviderConfiguration(SlowProvider.class.getName());
+ Path testFile = getCSVTestPath(conf);
+
+ int threads = 10;
+
+ AWSCredentialProviderList list =
createAWSCredentialProviderSet(testFile.toUri(), conf);
+
+ SlowProvider provider = (SlowProvider) list.getProviders().get(0);
+
+ ExecutorService pool = Executors.newFixedThreadPool(threads);
+
+ List<Future> results = new ArrayList<>(threads);
+
+ assertFalse(
+ "Provider not initialized. isInitialized should be false",
+ provider.isInitialized());
+ assertFalse(
+ "Provider not initialized. hasCredentials should be false",
+ provider.hasCredentials());
+ if (provider.getInitializationException() != null) {
+ throw new AssertionError(
+ "Provider not initialized. getInitializationException should return
null",
+ provider.getInitializationException());
+ }
+
+ for (int i = 0; i < threads; i++) {
+ results.add(pool.submit(() -> list.getCredentials()));
+ }
+
+ for (Future result : results) {
+ AWSCredentials credentials = (AWSCredentials) result.get();
+ assertEquals(credentials.getAWSAccessKeyId(), "expectedAccessKey");
+ assertEquals(credentials.getAWSSecretKey(), "expectedSecret");
+ }
+
+ pool.awaitTermination(10, TimeUnit.SECONDS);
+ pool.shutdown();
+
+ assertTrue(
+ "Provider initialized without errors. isInitialized should be true",
+ provider.isInitialized());
+ assertTrue(
+ "Provider initialized without errors. hasCredentials should be true",
+ provider.hasCredentials());
+ if (provider.getInitializationException() != null) {
+ throw new AssertionError(
+ "Provider initialized without errors. getInitializationException
should return null",
+ provider.getInitializationException());
+ }
+ }
+
+ /**
+ * Credential provider with error.
+ */
+ private static class ErrorProvider extends
AbstractSessionCredentialsProvider {
+
+ public ErrorProvider(@Nullable URI uri, Configuration conf) {
+ super(uri, conf);
+ }
+
+ @Override
+ protected AWSCredentials createCredentials(Configuration config) throws
IOException {
+ throw new IOException("expected error");
+ }
+ }
+
+ @Test
+ public void testConcurrentAuthenticationError() throws Throwable {
+ Configuration conf =
createProviderConfiguration(ErrorProvider.class.getName());
+ Path testFile = getCSVTestPath(conf);
+
+ int threads = 10;
+
+ AWSCredentialProviderList list =
createAWSCredentialProviderSet(testFile.toUri(), conf);
+ ErrorProvider provider = (ErrorProvider) list.getProviders().get(0);
+
+ ExecutorService pool = Executors.newFixedThreadPool(threads);
+
+ List<Future> results = new ArrayList<>(threads);
+
+ assertFalse("Provider not initialized. isInitialized should be false",
+ provider.isInitialized());
+ assertFalse("Provider not initialized. hasCredentials should be false",
+ provider.hasCredentials());
+ if (provider.getInitializationException() != null) {
+ throw new AssertionError(
+ "Provider not initialized. getInitializationException should return
null",
+ provider.getInitializationException());
+ }
+
+ for (int i = 0; i < threads; i++) {
+ results.add(pool.submit(() -> list.getCredentials()));
+ }
+
+ for (Future result : results) {
+ intercept(java.util.concurrent.ExecutionException.class,
Review Comment:
actually, i see we have an `interceptFuture` method which lets you specify
the class of the inner exception....
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AAWSCredentialsProvider.java:
##########
@@ -37,10 +42,13 @@
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.s3a.auth.AbstractSessionCredentialsProvider;
import org.apache.hadoop.fs.s3a.auth.AssumedRoleCredentialProvider;
import org.apache.hadoop.fs.s3a.auth.NoAuthWithAWSException;
import org.apache.hadoop.io.retry.RetryPolicy;
+import javax.annotation.Nullable;
Review Comment:
move up to just below the java.* imports
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AAWSCredentialsProvider.java:
##########
@@ -480,4 +488,149 @@ public void refresh() {
}
}
+ private static AWSCredentials expectedCredentials = new AWSCredentials() {
Review Comment:
make final; change case so the stylechecker is happy
--
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]
