Merge pull request #1427 from guobingkun/fix_issue_1016

Fix duplicate extension loading issue described in #1016

docs/content/configuration/index.md

@@ -23,7 +23,7 @@ Many of Druid's external dependencies can be plugged in as modules. Extensions c
 |--------|-----------|-------|
 |`druid.extensions.remoteRepositories`|This is a JSON Array list of remote repositories to load dependencies from. If this is not set to '[]', Druid will try to download extensions at the specified remote repository.|["http://repo1.maven.org/maven2/", "https://metamx.artifactoryonline.com/metamx/pub-libs-releases-local"]|
 |`druid.extensions.localRepository`|.  The way maven gets dependencies is that it downloads them to a "local repository" on your local disk and then collects the paths to each of the jars.  This specifies the directory to consider the "local repository". If this is set, remoteRepositories is not required.|`~/.m2/repository`|
-|`druid.extensions.coordinates`|This is a JSON array of "groupId:artifactId[:version]" maven coordinates. For artifacts without version specified, Druid will append the default version.|[]|
+|`druid.extensions.coordinates`|This is a JSON array of "groupId:artifactId[:version]" maven coordinates. For artifacts without version specified, Druid will append the default version. Notice: extensions explicitly specified in this property will have precedence over ones included in the classpath when Druid loads extensions. If there are duplicate extensions, Druid will only load ones explicitly specified here|[]|
 |`druid.extensions.defaultVersion`|Version to use for extension artifacts without version information.|`druid-server` artifact version.|
 |`druid.extensions.searchCurrentClassloader`|This is a boolean flag that determines if Druid will search the main classloader for extensions.  It defaults to true but can be turned off if you have reason to not automatically add all modules on the classpath.|true|
 

server/src/main/java/io/druid/initialization/Initialization.java

@@ -118,33 +118,52 @@ public class Initialization
   /**
    * Used for testing only
    */
-  protected static void clearLoadedModules()
+  static void clearLoadedModules()
   {
     extensionsMap.clear();
   }
 
+  /**
+   * Used for testing only
+   */
+  static Map<String, URLClassLoader> getLoadersMap()
+  {
+    return loadersMap;
+  }
+
+  /**
+   * Look for extension modules for the given class from both classpath and druid.extensions.coordinates.
+   * Extensions explicitly specified in druid.extensions.coordinates will be loaded first, if there is a duplicate
+   * extension from classpath, it will be ignored.
+   *
+   * @param config Extensions configuration
+   * @param clazz  The class of extension module (e.g., DruidModule)
+   *
+   * @return A collection that contains distinct extension modules
+   */
   public synchronized static <T> Collection<T> getFromExtensions(ExtensionsConfig config, Class<T> clazz)
   {
     final TeslaAether aether = getAetherClient(config);
-    Set<T> retVal = Sets.newHashSet();
+    final Set<T> retVal = Sets.newHashSet();
-
+    final Set<String> extensionNames = Sets.newHashSet();
-    if (config.searchCurrentClassloader()) {
-      for (T module : ServiceLoader.load(clazz, Initialization.class.getClassLoader())) {
-        log.info("Adding local module[%s]", module.getClass());
-        retVal.add(module);
-      }
-    }
 
     for (String coordinate : config.getCoordinates()) {
       log.info("Loading extension[%s] for class[%s]", coordinate, clazz.getName());
       try {
         URLClassLoader loader = getClassLoaderForCoordinates(aether, coordinate, config.getDefaultVersion());
 
-        final ServiceLoader<T> serviceLoader = ServiceLoader.load(clazz, loader);
+        for (T module : ServiceLoader.load(clazz, loader)) {
-
+          String moduleName = module.getClass().getCanonicalName();
-        for (T module : serviceLoader) {
+          if (moduleName == null) {
-          log.info("Adding extension module[%s] for class[%s]", module.getClass(), clazz.getName());
+            log.warn(
-          retVal.add(module);
+                "Extension module [%s] was ignored because it doesn't have a canonical name, is it a local or anonymous class?",
+                module.getClass().getName()
+            );
+          } else if (!extensionNames.contains(moduleName)) {
+            log.info("Adding remote extension module[%s] for class[%s]", moduleName, clazz.getName());
+            extensionNames.add(moduleName);
+            retVal.add(module);
+          }
         }
       }
       catch (Exception e) {
@@ -152,6 +171,22 @@ public class Initialization
       }
     }
 
+    if (config.searchCurrentClassloader()) {
+      for (T module : ServiceLoader.load(clazz, Initialization.class.getClassLoader())) {
+        String moduleName = module.getClass().getCanonicalName();
+        if (moduleName == null) {
+          log.warn(
+              "Extension module [%s] was ignored because it doesn't have a canonical name, is it a local or anonymous class?",
+              module.getClass().getName()
+          );
+        } else if (!extensionNames.contains(moduleName)) {
+          log.info("Adding local extension module[%s] for class[%s]", moduleName, clazz.getName());
+          extensionNames.add(moduleName);
+          retVal.add(module);
+        }
+      }
+    }
+
     // update the map with currently loaded modules
     extensionsMap.put(clazz, retVal);
 

server/src/test/java/io/druid/initialization/InitializationTest.java

@@ -19,6 +19,7 @@ package io.druid.initialization;
 
 import com.fasterxml.jackson.databind.Module;
 import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.api.client.util.Sets;
 import com.google.common.base.Function;
 import com.google.common.collect.Collections2;
 import com.google.common.collect.ImmutableList;
@@ -36,6 +37,7 @@ import org.junit.Test;
 import org.junit.runners.MethodSorters;
 
 import javax.annotation.Nullable;
+import java.net.URLClassLoader;
 import java.util.Collection;
 import java.util.List;
 import java.util.Set;
@@ -68,14 +70,14 @@ public class InitializationTest
     Injector startupInjector = GuiceInjectors.makeStartupInjector();
 
     Function<DruidModule, String> fnClassName = new Function<DruidModule, String>()
-        {
+    {
-          @Nullable
+      @Nullable
-          @Override
+      @Override
-          public String apply(@Nullable DruidModule input)
+      public String apply(@Nullable DruidModule input)
-          {
+      {
-            return input.getClass().getCanonicalName();
+        return input.getClass().getCanonicalName();
-          }
+      }
-        };
+    };
 
     Assert.assertFalse(
         "modules does not contain TestDruidModule",
@@ -96,7 +98,38 @@ public class InitializationTest
   }
 
   @Test
-  public void test04MakeInjectorWithModules() throws Exception
+  public void test04DuplicateClassLoaderExtensions() throws Exception
+  {
+    Initialization.getLoadersMap().put("xyz", (URLClassLoader) Initialization.class.getClassLoader());
+
+    Collection<DruidModule> modules = Initialization.getFromExtensions(
+        new ExtensionsConfig()
+        {
+          @Override
+          public List<String> getCoordinates()
+          {
+            return ImmutableList.of("xyz");
+          }
+
+          @Override
+          public List<String> getRemoteRepositories()
+          {
+            return ImmutableList.of();
+          }
+        }, DruidModule.class
+    );
+
+    Set<String> loadedModuleNames = Sets.newHashSet();
+    for (DruidModule module : modules) {
+      Assert.assertFalse("Duplicate extensions are loaded", loadedModuleNames.contains(module.getClass().getName()));
+      loadedModuleNames.add(module.getClass().getName());
+    }
+
+    Initialization.getLoadersMap().clear();
+  }
+
+  @Test
+  public void test05MakeInjectorWithModules() throws Exception
   {
     Injector startupInjector = GuiceInjectors.makeStartupInjector();
     Injector injector = Initialization.makeInjectorWithModules(