Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-10829

Iteration on CredentialProviderFactory.serviceLoader is thread-unsafe

    Details

    • Target Version/s:

      Description

      CredentialProviderFactory uses ServiceLoader framework to load CredentialProviderFactory

        private static final ServiceLoader<CredentialProviderFactory> serviceLoader =
            ServiceLoader.load(CredentialProviderFactory.class);
      

      The ServiceLoader framework does lazy initialization of services which makes it thread unsafe. If accessed from multiple threads, it is better to synchronize the access.
      Similar synchronization has been done while loading compression codec providers via HADOOP-8406.

      1. HADOOP-10829.patch
        1 kB
        Benoy Antony
      2. HADOOP-10829.patch
        1 kB
        Benoy Antony
      3. HADOOP-10829.003.patch
        2 kB
        Rakesh R

        Issue Links

          Activity

          Hide
          benoyantony Benoy Antony added a comment -

          Attaching the patch which synchronizes access to serviceLoader.
          Since there is no change in functionality, no tests are added.

          Show
          benoyantony Benoy Antony added a comment - Attaching the patch which synchronizes access to serviceLoader . Since there is no change in functionality, no tests are added.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12655807/HADOOP-10829.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

          org.apache.hadoop.metrics2.impl.TestMetricsSystemImpl
          org.apache.hadoop.fs.TestSymlinkLocalFSFileContext
          org.apache.hadoop.ipc.TestIPC
          org.apache.hadoop.fs.TestSymlinkLocalFSFileSystem

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4278//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4278//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12655807/HADOOP-10829.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.metrics2.impl.TestMetricsSystemImpl org.apache.hadoop.fs.TestSymlinkLocalFSFileContext org.apache.hadoop.ipc.TestIPC org.apache.hadoop.fs.TestSymlinkLocalFSFileSystem +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4278//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4278//console This message is automatically generated.
          Hide
          benoyantony Benoy Antony added a comment -

          Attaching patch which uses static initializer to preload the services. This will avoid lazy loading and hence prevent the need for synchronization during iteration.
          Similar approach was taken in HADOOP-10826.

          Show
          benoyantony Benoy Antony added a comment - Attaching patch which uses static initializer to preload the services. This will avoid lazy loading and hence prevent the need for synchronization during iteration. Similar approach was taken in HADOOP-10826 .
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12657346/HADOOP-10829.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

          org.apache.hadoop.fs.TestDFVariations
          org.apache.hadoop.ipc.TestIPC

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4348//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4348//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12657346/HADOOP-10829.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.fs.TestDFVariations org.apache.hadoop.ipc.TestIPC +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4348//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4348//console This message is automatically generated.
          Hide
          benoyantony Benoy Antony added a comment -

          Hi Uma Maheswara Rao G, Could you please review this patch and if appropriate, commit it ?

          Show
          benoyantony Benoy Antony added a comment - Hi Uma Maheswara Rao G , Could you please review this patch and if appropriate, commit it ?
          Hide
          benoyantony Benoy Antony added a comment -

          Vinayakumar B, could you please review this patch ?
          HADOOP-10826 was fixed in the same way.

          Show
          benoyantony Benoy Antony added a comment - Vinayakumar B , could you please review this patch ? HADOOP-10826 was fixed in the same way.
          Hide
          hadoopqa Hadoop QA added a comment -



          -1 overall



          Vote Subsystem Runtime Comment
          0 pre-patch 14m 34s Pre-patch trunk compilation is healthy.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 tests included 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
          +1 javac 7m 28s There were no new javac warning messages.
          -1 javadoc 9m 55s The applied patch generated 50 additional warning messages.
          +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings.
          +1 checkstyle 1m 12s There were no new checkstyle issues.
          -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix.
          +1 install 1m 36s mvn install still works.
          +1 eclipse:eclipse 0m 32s The patch built with eclipse:eclipse.
          +1 findbugs 1m 43s The patch does not introduce any new Findbugs (version 2.0.3) warnings.
          +1 common tests 24m 7s Tests passed in hadoop-common.
              61m 34s  



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12657346/HADOOP-10829.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / 6ae2a0d
          javadoc https://builds.apache.org/job/PreCommit-HADOOP-Build/6436/artifact/patchprocess/diffJavadocWarnings.txt
          whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/6436/artifact/patchprocess/whitespace.txt
          hadoop-common test log https://builds.apache.org/job/PreCommit-HADOOP-Build/6436/artifact/patchprocess/testrun_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/6436/testReport/
          Java 1.7.0_55
          uname Linux asf903.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/6436/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 14m 34s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. -1 tests included 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac 7m 28s There were no new javac warning messages. -1 javadoc 9m 55s The applied patch generated 50 additional warning messages. +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings. +1 checkstyle 1m 12s There were no new checkstyle issues. -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 install 1m 36s mvn install still works. +1 eclipse:eclipse 0m 32s The patch built with eclipse:eclipse. +1 findbugs 1m 43s The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 common tests 24m 7s Tests passed in hadoop-common.     61m 34s   Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12657346/HADOOP-10829.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 6ae2a0d javadoc https://builds.apache.org/job/PreCommit-HADOOP-Build/6436/artifact/patchprocess/diffJavadocWarnings.txt whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/6436/artifact/patchprocess/whitespace.txt hadoop-common test log https://builds.apache.org/job/PreCommit-HADOOP-Build/6436/artifact/patchprocess/testrun_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/6436/testReport/ Java 1.7.0_55 uname Linux asf903.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/6436/console This message was automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -



          -1 overall



          Vote Subsystem Runtime Comment
          0 pre-patch 19m 42s Pre-patch trunk compilation is healthy.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 tests included 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
          +1 javac 9m 32s There were no new javac warning messages.
          +1 javadoc 10m 54s There were no new javadoc warning messages.
          +1 release audit 0m 26s The applied patch does not increase the total number of release audit warnings.
          +1 checkstyle 1m 11s There were no new checkstyle issues.
          -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix.
          +1 install 1m 33s mvn install still works.
          +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
          +1 findbugs 1m 53s The patch does not introduce any new Findbugs (version 3.0.0) warnings.
          -1 common tests 23m 16s Tests failed in hadoop-common.
              69m 3s  



          Reason Tests
          Failed unit tests hadoop.ipc.TestSaslRPC



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12657346/HADOOP-10829.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / 15a557f
          whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/7644/artifact/patchprocess/whitespace.txt
          hadoop-common test log https://builds.apache.org/job/PreCommit-HADOOP-Build/7644/artifact/patchprocess/testrun_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/7644/testReport/
          Java 1.7.0_55
          uname Linux asf909.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/7644/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 19m 42s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. -1 tests included 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac 9m 32s There were no new javac warning messages. +1 javadoc 10m 54s There were no new javadoc warning messages. +1 release audit 0m 26s The applied patch does not increase the total number of release audit warnings. +1 checkstyle 1m 11s There were no new checkstyle issues. -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 install 1m 33s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 1m 53s The patch does not introduce any new Findbugs (version 3.0.0) warnings. -1 common tests 23m 16s Tests failed in hadoop-common.     69m 3s   Reason Tests Failed unit tests hadoop.ipc.TestSaslRPC Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12657346/HADOOP-10829.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 15a557f whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/7644/artifact/patchprocess/whitespace.txt hadoop-common test log https://builds.apache.org/job/PreCommit-HADOOP-Build/7644/artifact/patchprocess/testrun_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/7644/testReport/ Java 1.7.0_55 uname Linux asf909.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/7644/console This message was automatically generated.
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          Moving out all non-critical / non-blocker issues that didn't make it out of 2.7.2 into 2.7.3. Please revert back if you disagree.

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - Moving out all non-critical / non-blocker issues that didn't make it out of 2.7.2 into 2.7.3. Please revert back if you disagree.
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          2.7.3 is under release process, changing target-version to 2.7.4.

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - 2.7.3 is under release process, changing target-version to 2.7.4.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 0s Docker mode activated.
          -1 patch 0m 5s HADOOP-10829 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help.



          Subsystem Report/Notes
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12657346/HADOOP-10829.patch
          JIRA Issue HADOOP-10829
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10291/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. -1 patch 0m 5s HADOOP-10829 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help. Subsystem Report/Notes JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12657346/HADOOP-10829.patch JIRA Issue HADOOP-10829 Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10291/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          rakeshr Rakesh R added a comment -

          Thanks Benoy Antony for the fix.

          +1 (non-binding) for the fix. I've rebased the patch on latest trunk code, please someone help in reviewing the changes. Thanks!

          Show
          rakeshr Rakesh R added a comment - Thanks Benoy Antony for the fix. +1 (non-binding) for the fix. I've rebased the patch on latest trunk code, please someone help in reviewing the changes. Thanks!
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 13s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
          +1 mvninstall 7m 10s trunk passed
          +1 compile 7m 58s trunk passed
          +1 checkstyle 0m 26s trunk passed
          +1 mvnsite 1m 1s trunk passed
          +1 mvneclipse 0m 14s trunk passed
          +1 findbugs 1m 26s trunk passed
          +1 javadoc 0m 48s trunk passed
          +1 mvninstall 0m 40s the patch passed
          +1 compile 7m 58s the patch passed
          +1 javac 7m 58s the patch passed
          +1 checkstyle 0m 23s the patch passed
          +1 mvnsite 0m 57s the patch passed
          +1 mvneclipse 0m 13s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 39s the patch passed
          +1 javadoc 0m 44s the patch passed
          +1 unit 8m 44s hadoop-common in the patch passed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          42m 16s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-10829
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12835546/HADOOP-10829.003.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 0097c104cba3 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 4e403de
          Default Java 1.8.0_101
          findbugs v3.0.0
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10908/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10908/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 13s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 7m 10s trunk passed +1 compile 7m 58s trunk passed +1 checkstyle 0m 26s trunk passed +1 mvnsite 1m 1s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 1m 26s trunk passed +1 javadoc 0m 48s trunk passed +1 mvninstall 0m 40s the patch passed +1 compile 7m 58s the patch passed +1 javac 7m 58s the patch passed +1 checkstyle 0m 23s the patch passed +1 mvnsite 0m 57s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 39s the patch passed +1 javadoc 0m 44s the patch passed +1 unit 8m 44s hadoop-common in the patch passed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 42m 16s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-10829 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12835546/HADOOP-10829.003.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 0097c104cba3 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 4e403de Default Java 1.8.0_101 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10908/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10908/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          lmccay Larry McCay added a comment -

          Rakesh R - I am inclined to prefer the approach used in KeyProviderFactory instead which avoids the lazy loading by iterating over the providers in a static initialization block. This avoids the need for synchronization during each call to getProviders for every client in a JVM.

            public abstract KeyProvider createProvider(URI providerName,
                                                       Configuration conf
                                                       ) throws IOException;
          
            private static final ServiceLoader<KeyProviderFactory> serviceLoader =
                ServiceLoader.load(KeyProviderFactory.class,
                    KeyProviderFactory.class.getClassLoader());
          
            // Iterate through the serviceLoader to avoid lazy loading.
            // Lazy loading would require synchronization in concurrent use cases.
            static {
              Iterator<KeyProviderFactory> iterServices = serviceLoader.iterator();
              while (iterServices.hasNext()) {
                iterServices.next();
              }
            }
            
            public static List<KeyProvider> getProviders(Configuration conf
                                                         ) throws IOException {
              List<KeyProvider> result = new ArrayList<KeyProvider>();
              for(String path: conf.getStringCollection(KEY_PROVIDER_PATH)) {
                try {
                  URI uri = new URI(path);
                  KeyProvider kp = get(uri, conf);
                  if (kp != null) {
                    result.add(kp);
                  } else {
                    throw new IOException("No KeyProviderFactory for " + uri + " in " +
                        KEY_PROVIDER_PATH);
                  }
                } catch (URISyntaxException error) {
                  throw new IOException("Bad configuration of " + KEY_PROVIDER_PATH +
                      " at " + path, error);
                }
              }
              return result;
            }
          

          What do you think?

          Show
          lmccay Larry McCay added a comment - Rakesh R - I am inclined to prefer the approach used in KeyProviderFactory instead which avoids the lazy loading by iterating over the providers in a static initialization block. This avoids the need for synchronization during each call to getProviders for every client in a JVM. public abstract KeyProvider createProvider(URI providerName, Configuration conf ) throws IOException; private static final ServiceLoader<KeyProviderFactory> serviceLoader = ServiceLoader.load(KeyProviderFactory.class, KeyProviderFactory.class.getClassLoader()); // Iterate through the serviceLoader to avoid lazy loading. // Lazy loading would require synchronization in concurrent use cases. static { Iterator<KeyProviderFactory> iterServices = serviceLoader.iterator(); while (iterServices.hasNext()) { iterServices.next(); } } public static List<KeyProvider> getProviders(Configuration conf ) throws IOException { List<KeyProvider> result = new ArrayList<KeyProvider>(); for ( String path: conf.getStringCollection(KEY_PROVIDER_PATH)) { try { URI uri = new URI(path); KeyProvider kp = get(uri, conf); if (kp != null ) { result.add(kp); } else { throw new IOException( "No KeyProviderFactory for " + uri + " in " + KEY_PROVIDER_PATH); } } catch (URISyntaxException error) { throw new IOException( "Bad configuration of " + KEY_PROVIDER_PATH + " at " + path, error); } } return result; } What do you think?
          Hide
          rakeshr Rakesh R added a comment -

          Thanks Larry McCay,

          I am inclined to prefer the approach used in KeyProviderFactory

          IIUC, you are suggesting to the use the following way.

          +  // Iterate through the serviceLoader to avoid lazy loading.
          +  // Lazy loading would require synchronization in concurrent use cases.
          +  static {
          +    Iterator<CredentialProviderFactory> iterServices = serviceLoader.iterator();
          +    while (iterServices.hasNext()) {
          +      iterServices.next();
          +    }
          +  }
          +
          

          I had uploaded similar approach in the recent patch HADOOP-10829.003.patch, is that ok?

          Show
          rakeshr Rakesh R added a comment - Thanks Larry McCay , I am inclined to prefer the approach used in KeyProviderFactory IIUC, you are suggesting to the use the following way. + // Iterate through the serviceLoader to avoid lazy loading. + // Lazy loading would require synchronization in concurrent use cases. + static { + Iterator<CredentialProviderFactory> iterServices = serviceLoader.iterator(); + while (iterServices.hasNext()) { + iterServices.next(); + } + } + I had uploaded similar approach in the recent patch HADOOP-10829.003.patch , is that ok?
          Hide
          lmccay Larry McCay added a comment -

          Sorry for the ridiculous delay in my response.
          Here is my +1.

          Thanks!

          Show
          lmccay Larry McCay added a comment - Sorry for the ridiculous delay in my response. Here is my +1. Thanks!
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 2m 58s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
          +1 mvninstall 12m 20s trunk passed
          +1 compile 14m 45s trunk passed
          +1 checkstyle 0m 33s trunk passed
          +1 mvnsite 1m 4s trunk passed
          +1 findbugs 1m 15s trunk passed
          +1 javadoc 0m 50s trunk passed
          +1 mvninstall 0m 35s the patch passed
          +1 compile 9m 45s the patch passed
          +1 javac 9m 45s the patch passed
          +1 checkstyle 0m 36s the patch passed
          +1 mvnsite 1m 0s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 26s the patch passed
          +1 javadoc 0m 53s the patch passed
          -1 unit 6m 48s hadoop-common in the patch failed.
          +1 asflicense 0m 32s The patch does not generate ASF License warnings.
          57m 18s



          Reason Tests
          Failed junit tests hadoop.fs.sftp.TestSFTPFileSystem



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HADOOP-10829
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12835546/HADOOP-10829.003.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 03d68f7d7a2f 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / fa1aaee
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12693/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12693/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12693/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 2m 58s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 12m 20s trunk passed +1 compile 14m 45s trunk passed +1 checkstyle 0m 33s trunk passed +1 mvnsite 1m 4s trunk passed +1 findbugs 1m 15s trunk passed +1 javadoc 0m 50s trunk passed +1 mvninstall 0m 35s the patch passed +1 compile 9m 45s the patch passed +1 javac 9m 45s the patch passed +1 checkstyle 0m 36s the patch passed +1 mvnsite 1m 0s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 26s the patch passed +1 javadoc 0m 53s the patch passed -1 unit 6m 48s hadoop-common in the patch failed. +1 asflicense 0m 32s The patch does not generate ASF License warnings. 57m 18s Reason Tests Failed junit tests hadoop.fs.sftp.TestSFTPFileSystem Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HADOOP-10829 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12835546/HADOOP-10829.003.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 03d68f7d7a2f 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / fa1aaee Default Java 1.8.0_131 findbugs v3.1.0-RC1 unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12693/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12693/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12693/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jnp Jitendra Nath Pandey added a comment -

          Rakesh R, are you planning to commit this, given there is a +1?

          Show
          jnp Jitendra Nath Pandey added a comment - Rakesh R , are you planning to commit this, given there is a +1?
          Hide
          jnp Jitendra Nath Pandey added a comment -

          I will commit this shortly.

          Show
          jnp Jitendra Nath Pandey added a comment - I will commit this shortly.
          Hide
          jnp Jitendra Nath Pandey added a comment -

          I have committed this to trunk and branch-2.

          Show
          jnp Jitendra Nath Pandey added a comment - I have committed this to trunk and branch-2.
          Hide
          jnp Jitendra Nath Pandey added a comment -

          Thanks to Benoy and Rakesh for the patches.

          Show
          jnp Jitendra Nath Pandey added a comment - Thanks to Benoy and Rakesh for the patches.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11984 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11984/)
          HADOOP-10829. Iteration on CredentialProviderFactory.serviceLoader is (jitendra: rev f1efa14fc676641fa15c11d3147e3ad948b084e9)

          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/CredentialProviderFactory.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11984 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11984/ ) HADOOP-10829 . Iteration on CredentialProviderFactory.serviceLoader is (jitendra: rev f1efa14fc676641fa15c11d3147e3ad948b084e9) (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/CredentialProviderFactory.java
          Hide
          zhz Zhe Zhang added a comment -

          Thanks for the fix Benoy Antony. Given this is a security bug fix, I just backported to branch-2.8 and branch-2.7

          Show
          zhz Zhe Zhang added a comment - Thanks for the fix Benoy Antony . Given this is a security bug fix, I just backported to branch-2.8 and branch-2.7
          Hide
          djp Junping Du added a comment -

          Merge to branch-2.8.2 too given 2.7.4 is released and we won't want any bug regression here.

          Show
          djp Junping Du added a comment - Merge to branch-2.8.2 too given 2.7.4 is released and we won't want any bug regression here.

            People

            • Assignee:
              benoyantony Benoy Antony
              Reporter:
              benoyantony Benoy Antony
            • Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development