From 65facc935d7eafc326a1058e4893b3e905fcc3ea Mon Sep 17 00:00:00 2001 From: Himadri Singh Date: Fri, 13 Dec 2013 13:05:57 +0530 Subject: [PATCH 1/7] Do not load extensions on each call --- .../druid/initialization/Initialization.java | 14 +++++ .../java/io/druid/server/StatusResource.java | 54 ++++++++++++++++--- 2 files changed, 61 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/io/druid/initialization/Initialization.java b/server/src/main/java/io/druid/initialization/Initialization.java index bb30718a33f..4051f70a7c8 100644 --- a/server/src/main/java/io/druid/initialization/Initialization.java +++ b/server/src/main/java/io/druid/initialization/Initialization.java @@ -101,6 +101,16 @@ public class Initialization "io.druid", "com.metamx.druid" ); + private static List loadedDruidModules = Lists.newArrayList(); + + /** + * @return List of {@link DruidModule}, once loaded. + */ + public static List getLoadedDruidModules() + { + return loadedDruidModules; + } + public synchronized static List getFromExtensions(ExtensionsConfig config, Class clazz) { @@ -131,6 +141,10 @@ public class Initialization } } + if (clazz.equals(DruidModule.class)){ + loadedDruidModules.addAll(retVal); + } + return retVal; } diff --git a/server/src/main/java/io/druid/server/StatusResource.java b/server/src/main/java/io/druid/server/StatusResource.java index d6e5371f0c3..9a281478524 100644 --- a/server/src/main/java/io/druid/server/StatusResource.java +++ b/server/src/main/java/io/druid/server/StatusResource.java @@ -19,12 +19,16 @@ package io.druid.server; +import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.annotation.JsonProperty; +import io.druid.initialization.DruidModule; import io.druid.initialization.Initialization; import javax.ws.rs.GET; import javax.ws.rs.Path; import javax.ws.rs.Produces; +import java.util.ArrayList; +import java.util.List; /** */ @@ -42,20 +46,42 @@ public class StatusResource { return new Status( Initialization.class.getPackage().getImplementationVersion(), + getExtensionVersions(), new Memory(Runtime.getRuntime()) ); } + /** + * Load the unique extensions and return their implementation-versions + * + * @return map of extensions loaded with their respective implementation versions. + */ + private static List getExtensionVersions() + { + final List druidModules = Initialization.getLoadedDruidModules(); + + List moduleVersions = new ArrayList<>(); + for (DruidModule module : druidModules) { + String artifact = module.getClass().getPackage().getImplementationTitle(); + String version = module.getClass().getPackage().getImplementationVersion(); + + moduleVersions.add(new ModuleVersion(module.getClass().getCanonicalName(), artifact, version)); + } + return moduleVersions; + } + public static class Status { final String version; + final List modules; final Memory memory; public Status( - String version, Memory memory + String version, List modules, Memory memory ) { this.version = version; + this.modules = modules; this.memory = memory; } @@ -65,6 +91,12 @@ public class StatusResource return version; } + @JsonProperty + public List getModules() + { + return modules; + } + @JsonProperty public Memory getMemory() { @@ -75,21 +107,29 @@ public class StatusResource public String toString() { final String NL = System.getProperty("line.separator"); - return String.format("Druid version - %s", version) + NL; + StringBuilder output = new StringBuilder(); + output.append(String.format("Druid version - %s", version)).append(NL).append(NL); + + if (modules.size() > 0) { + output.append("Registered Druid Modules").append(NL); + } else { + output.append("No Druid Modules loaded !"); + } + + for (ModuleVersion moduleVersion : modules) { + output.append(moduleVersion).append(NL); + } + return output.toString(); } } + @JsonInclude(JsonInclude.Include.NON_NULL) public static class ModuleVersion { final String name; final String artifact; final String version; - public ModuleVersion(String name) - { - this(name, "", ""); - } - public ModuleVersion(String name, String artifact, String version) { this.name = name; From 3752122a7b4dcf53866c958b4fdf9ad17dcfb0ad Mon Sep 17 00:00:00 2001 From: Himadri Singh Date: Mon, 16 Dec 2013 15:16:28 +0530 Subject: [PATCH 2/7] implementing comments from review --- .../druid/initialization/Initialization.java | 22 +++---- .../java/io/druid/server/StatusResource.java | 60 ++++++++----------- services/src/main/java/io/druid/cli/Main.java | 3 +- .../src/main/java/io/druid/cli/Version.java | 2 +- 4 files changed, 40 insertions(+), 47 deletions(-) diff --git a/server/src/main/java/io/druid/initialization/Initialization.java b/server/src/main/java/io/druid/initialization/Initialization.java index 4051f70a7c8..48fe618a5b8 100644 --- a/server/src/main/java/io/druid/initialization/Initialization.java +++ b/server/src/main/java/io/druid/initialization/Initialization.java @@ -84,6 +84,7 @@ import java.net.URI; import java.net.URISyntaxException; import java.net.URL; import java.net.URLClassLoader; +import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.Map; @@ -101,21 +102,23 @@ public class Initialization "io.druid", "com.metamx.druid" ); - private static List loadedDruidModules = Lists.newArrayList(); + private final static Map extensionsMap = Maps.newHashMap(); /** - * @return List of {@link DruidModule}, once loaded. - */ - public static List getLoadedDruidModules() + * @param clazz Module class + * @param + * @return Returns the set of modules loaded. + */ + public static Set getLoadedModules(Class clazz) { - return loadedDruidModules; + return extensionsMap.get(clazz); } - public synchronized static List getFromExtensions(ExtensionsConfig config, Class clazz) + public synchronized static Collection getFromExtensions(ExtensionsConfig config, Class clazz) { final TeslaAether aether = getAetherClient(config); - List retVal = Lists.newArrayList(); + Set retVal = Sets.newHashSet(); if (config.searchCurrentClassloader()) { for (T module : ServiceLoader.load(clazz, Initialization.class.getClassLoader())) { @@ -141,9 +144,8 @@ public class Initialization } } - if (clazz.equals(DruidModule.class)){ - loadedDruidModules.addAll(retVal); - } + // update the map with currently loaded modules + extensionsMap.put(clazz, retVal); return retVal; } diff --git a/server/src/main/java/io/druid/server/StatusResource.java b/server/src/main/java/io/druid/server/StatusResource.java index 9a281478524..7d8ec62d611 100644 --- a/server/src/main/java/io/druid/server/StatusResource.java +++ b/server/src/main/java/io/druid/server/StatusResource.java @@ -29,6 +29,7 @@ import javax.ws.rs.Path; import javax.ws.rs.Produces; import java.util.ArrayList; import java.util.List; +import java.util.Set; /** */ @@ -39,35 +40,7 @@ public class StatusResource @Produces("application/json") public Status doGet() { - return getStatus(); - } - - public static Status getStatus() - { - return new Status( - Initialization.class.getPackage().getImplementationVersion(), - getExtensionVersions(), - new Memory(Runtime.getRuntime()) - ); - } - - /** - * Load the unique extensions and return their implementation-versions - * - * @return map of extensions loaded with their respective implementation versions. - */ - private static List getExtensionVersions() - { - final List druidModules = Initialization.getLoadedDruidModules(); - - List moduleVersions = new ArrayList<>(); - for (DruidModule module : druidModules) { - String artifact = module.getClass().getPackage().getImplementationTitle(); - String version = module.getClass().getPackage().getImplementationVersion(); - - moduleVersions.add(new ModuleVersion(module.getClass().getCanonicalName(), artifact, version)); - } - return moduleVersions; + return new Status(); } public static class Status @@ -76,13 +49,11 @@ public class StatusResource final List modules; final Memory memory; - public Status( - String version, List modules, Memory memory - ) + public Status() { - this.version = version; - this.modules = modules; - this.memory = memory; + this.version = Status.class.getPackage().getImplementationVersion(); + this.modules = getExtensionVersions(); + this.memory = new Memory(Runtime.getRuntime()); } @JsonProperty @@ -121,6 +92,25 @@ public class StatusResource } return output.toString(); } + + /** + * Load the unique extensions and return their implementation-versions + * + * @return map of extensions loaded with their respective implementation versions. + */ + private List getExtensionVersions() + { + final Set druidModules = Initialization.getLoadedModules(DruidModule.class); + List moduleVersions = new ArrayList<>(); + for (DruidModule module : druidModules) { + String artifact = module.getClass().getPackage().getImplementationTitle(); + String version = module.getClass().getPackage().getImplementationVersion(); + + moduleVersions.add(new ModuleVersion(module.getClass().getCanonicalName(), artifact, version)); + } + return moduleVersions; + } + } @JsonInclude(JsonInclude.Include.NON_NULL) diff --git a/services/src/main/java/io/druid/cli/Main.java b/services/src/main/java/io/druid/cli/Main.java index 7e9a633e8ed..8fc06ffcdc5 100644 --- a/services/src/main/java/io/druid/cli/Main.java +++ b/services/src/main/java/io/druid/cli/Main.java @@ -30,6 +30,7 @@ import io.druid.server.initialization.ExtensionsConfig; import org.apache.log4j.Level; import org.apache.log4j.Logger; +import java.util.Collection; import java.util.List; /** @@ -75,7 +76,7 @@ public class Main final Injector injector = Initialization.makeStartupInjector(); final ExtensionsConfig config = injector.getInstance(ExtensionsConfig.class); - final List extensionCommands = Initialization.getFromExtensions(config, CliCommandCreator.class); + final Collection extensionCommands = Initialization.getFromExtensions(config, CliCommandCreator.class); for (CliCommandCreator creator : extensionCommands) { creator.addCommands(builder); diff --git a/services/src/main/java/io/druid/cli/Version.java b/services/src/main/java/io/druid/cli/Version.java index 9212a3cb800..210591e81e6 100644 --- a/services/src/main/java/io/druid/cli/Version.java +++ b/services/src/main/java/io/druid/cli/Version.java @@ -31,6 +31,6 @@ public class Version implements Runnable @Override public void run() { - System.out.println(StatusResource.getStatus()); + System.out.println(new StatusResource.Status()); } } From c58fbfe40c5b4fc8a1c92f9475aafe07907ca1a2 Mon Sep 17 00:00:00 2001 From: Himadri Singh Date: Thu, 19 Dec 2013 10:49:38 +0530 Subject: [PATCH 3/7] minor fixes --- .../main/java/io/druid/initialization/Initialization.java | 7 +++++-- server/src/main/java/io/druid/server/StatusResource.java | 3 +-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/io/druid/initialization/Initialization.java b/server/src/main/java/io/druid/initialization/Initialization.java index 48fe618a5b8..5e8e0461202 100644 --- a/server/src/main/java/io/druid/initialization/Initialization.java +++ b/server/src/main/java/io/druid/initialization/Initialization.java @@ -111,9 +111,12 @@ public class Initialization */ public static Set getLoadedModules(Class clazz) { - return extensionsMap.get(clazz); + Set retVal = extensionsMap.get(clazz); + if (retVal == null) { + return Sets.newHashSet(); + } + return retVal; } - public synchronized static Collection getFromExtensions(ExtensionsConfig config, Class clazz) { diff --git a/server/src/main/java/io/druid/server/StatusResource.java b/server/src/main/java/io/druid/server/StatusResource.java index 7d8ec62d611..643d4d4fd74 100644 --- a/server/src/main/java/io/druid/server/StatusResource.java +++ b/server/src/main/java/io/druid/server/StatusResource.java @@ -148,7 +148,7 @@ public class StatusResource @Override public String toString() { - if (artifact.isEmpty()) { + if (artifact == null || artifact.isEmpty()) { return String.format(" - %s ", name); } else { return String.format(" - %s (%s-%s)", name, artifact, version); @@ -194,6 +194,5 @@ public class StatusResource { return usedMemory; } - } } From 52746b8ea6d15e1814adbc657c7d64ee1af2af36 Mon Sep 17 00:00:00 2001 From: Hagen Rother Date: Thu, 19 Dec 2013 06:54:04 +0100 Subject: [PATCH 4/7] fix hadoop intake's parser exception catching (was too specific) --- .../main/java/io/druid/indexer/HadoopDruidIndexerMapper.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/indexing-hadoop/src/main/java/io/druid/indexer/HadoopDruidIndexerMapper.java b/indexing-hadoop/src/main/java/io/druid/indexer/HadoopDruidIndexerMapper.java index 9f1bd030284..2eedaf76d31 100644 --- a/indexing-hadoop/src/main/java/io/druid/indexer/HadoopDruidIndexerMapper.java +++ b/indexing-hadoop/src/main/java/io/druid/indexer/HadoopDruidIndexerMapper.java @@ -62,7 +62,7 @@ public abstract class HadoopDruidIndexerMapper extends Mapper< try { inputRow = parser.parse(value.toString()); } - catch (IllegalArgumentException e) { + catch (Exception e) { if (config.isIgnoreInvalidRows()) { context.getCounter(HadoopDruidIndexerConfig.IndexJobCounters.INVALID_ROW_COUNTER).increment(1); return; // we're ignoring this invalid row From b1a9ecc1cfa44ac38120b886a6e9f9f43dcc3baf Mon Sep 17 00:00:00 2001 From: Himadri Singh Date: Thu, 19 Dec 2013 11:42:22 +0530 Subject: [PATCH 5/7] StatusResourceTest misses testing reloading of modules currently --- .../io/druid/server/StatusResourceTest.java | 72 +++++++++++++++++++ .../java/io/druid/server/TestDruidModule.java | 44 ++++++++++++ .../io.druid.initialization.DruidModule | 1 + 3 files changed, 117 insertions(+) create mode 100644 server/src/test/java/io/druid/server/StatusResourceTest.java create mode 100644 server/src/test/java/io/druid/server/TestDruidModule.java create mode 100644 server/src/test/resources/META-INF/services/io.druid.initialization.DruidModule diff --git a/server/src/test/java/io/druid/server/StatusResourceTest.java b/server/src/test/java/io/druid/server/StatusResourceTest.java new file mode 100644 index 00000000000..380afcdab9b --- /dev/null +++ b/server/src/test/java/io/druid/server/StatusResourceTest.java @@ -0,0 +1,72 @@ +/* + * Druid - a distributed column store. + * Copyright (C) 2012, 2013 Metamarkets Group Inc. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +package io.druid.server; + +import static io.druid.server.StatusResource.ModuleVersion; + +import com.google.inject.Injector; +import io.druid.initialization.DruidModule; +import io.druid.initialization.Initialization; +import io.druid.server.initialization.ExtensionsConfig; +import junit.framework.Assert; +import org.junit.Test; + +import java.util.Collection; +import java.util.List; + +/** + */ +public class StatusResourceTest +{ + + private Collection loadTestModule() + { + Injector baseInjector = Initialization.makeStartupInjector(); + return Initialization.getFromExtensions(baseInjector.getInstance(ExtensionsConfig.class), DruidModule.class); + } + + @Test + public void testLoadedModules() + { + final StatusResource resource = new StatusResource(); + List statusResourceModuleList; + + statusResourceModuleList = resource.doGet().getModules(); + Assert.assertEquals("No Modules should be loaded currently!", statusResourceModuleList.size(), 0); + + Collection modules = loadTestModule(); + statusResourceModuleList = resource.doGet().getModules(); + + Assert.assertEquals("Status should have all modules loaded!", statusResourceModuleList.size(), modules.size()); + + for (DruidModule module : modules) { + String moduleName = module.getClass().getCanonicalName(); + + boolean contains = Boolean.FALSE; + for (ModuleVersion version : statusResourceModuleList) { + if (version.getName().equals(moduleName)) { + contains = Boolean.TRUE; + } + } + Assert.assertTrue("Status resource should contains module " + moduleName, contains); + } + } + +} diff --git a/server/src/test/java/io/druid/server/TestDruidModule.java b/server/src/test/java/io/druid/server/TestDruidModule.java new file mode 100644 index 00000000000..0a428d808cd --- /dev/null +++ b/server/src/test/java/io/druid/server/TestDruidModule.java @@ -0,0 +1,44 @@ +/* + * Druid - a distributed column store. + * Copyright (C) 2012, 2013 Metamarkets Group Inc. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +package io.druid.server; + +import com.fasterxml.jackson.databind.Module; +import com.google.common.collect.ImmutableList; +import com.google.inject.Binder; +import io.druid.initialization.DruidModule; + +import java.util.List; + +/** + */ +public class TestDruidModule implements DruidModule +{ + @Override + public List getJacksonModules() + { + return ImmutableList.of(); + } + + @Override + public void configure(Binder binder) + { + // Do nothing + } +} diff --git a/server/src/test/resources/META-INF/services/io.druid.initialization.DruidModule b/server/src/test/resources/META-INF/services/io.druid.initialization.DruidModule new file mode 100644 index 00000000000..7acb2959046 --- /dev/null +++ b/server/src/test/resources/META-INF/services/io.druid.initialization.DruidModule @@ -0,0 +1 @@ +io.druid.server.TestDruidModule From cd1e7cc65d1184628e942fb8714f6468ff43290c Mon Sep 17 00:00:00 2001 From: Himadri Singh Date: Thu, 19 Dec 2013 12:01:18 +0530 Subject: [PATCH 6/7] Updated test --- .../io/druid/server/StatusResourceTest.java | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/server/src/test/java/io/druid/server/StatusResourceTest.java b/server/src/test/java/io/druid/server/StatusResourceTest.java index 380afcdab9b..2079c36b74d 100644 --- a/server/src/test/java/io/druid/server/StatusResourceTest.java +++ b/server/src/test/java/io/druid/server/StatusResourceTest.java @@ -19,8 +19,6 @@ package io.druid.server; -import static io.druid.server.StatusResource.ModuleVersion; - import com.google.inject.Injector; import io.druid.initialization.DruidModule; import io.druid.initialization.Initialization; @@ -30,6 +28,9 @@ import org.junit.Test; import java.util.Collection; import java.util.List; +import java.util.Set; + +import static io.druid.server.StatusResource.ModuleVersion; /** */ @@ -49,7 +50,10 @@ public class StatusResourceTest List statusResourceModuleList; statusResourceModuleList = resource.doGet().getModules(); - Assert.assertEquals("No Modules should be loaded currently!", statusResourceModuleList.size(), 0); + Assert.assertEquals( + "No Modules should be loaded currently! " + statusResourceModuleList, + statusResourceModuleList.size(), 0 + ); Collection modules = loadTestModule(); statusResourceModuleList = resource.doGet().getModules(); @@ -65,8 +69,17 @@ public class StatusResourceTest contains = Boolean.TRUE; } } - Assert.assertTrue("Status resource should contains module " + moduleName, contains); + Assert.assertTrue("Status resource should contain module " + moduleName, contains); } + + /* + * StatusResource only uses Initialization.getLoadedModules + */ + for (int i = 0; i < 5; i++) { + Set loadedModules = Initialization.getLoadedModules(DruidModule.class); + Assert.assertEquals("Set from loaded module should be same!", loadedModules, modules); + } + } } From 818aba458fcb247fd70ffb1be55d8b21d3f16788 Mon Sep 17 00:00:00 2001 From: Himadri Singh Date: Thu, 19 Dec 2013 12:14:06 +0530 Subject: [PATCH 7/7] Moved TestDruidModule --- .../io/druid/server/StatusResourceTest.java | 18 ++++++++ .../java/io/druid/server/TestDruidModule.java | 44 ------------------- .../io.druid.initialization.DruidModule | 2 +- 3 files changed, 19 insertions(+), 45 deletions(-) delete mode 100644 server/src/test/java/io/druid/server/TestDruidModule.java diff --git a/server/src/test/java/io/druid/server/StatusResourceTest.java b/server/src/test/java/io/druid/server/StatusResourceTest.java index 2079c36b74d..cff0fb4618e 100644 --- a/server/src/test/java/io/druid/server/StatusResourceTest.java +++ b/server/src/test/java/io/druid/server/StatusResourceTest.java @@ -19,6 +19,9 @@ package io.druid.server; +import com.fasterxml.jackson.databind.Module; +import com.google.common.collect.ImmutableList; +import com.google.inject.Binder; import com.google.inject.Injector; import io.druid.initialization.DruidModule; import io.druid.initialization.Initialization; @@ -79,7 +82,22 @@ public class StatusResourceTest Set loadedModules = Initialization.getLoadedModules(DruidModule.class); Assert.assertEquals("Set from loaded module should be same!", loadedModules, modules); } + } + public static class TestDruidModule implements DruidModule + { + @Override + public List getJacksonModules() + { + return ImmutableList.of(); + } + + @Override + public void configure(Binder binder) + { + // Do nothing + } } } + diff --git a/server/src/test/java/io/druid/server/TestDruidModule.java b/server/src/test/java/io/druid/server/TestDruidModule.java deleted file mode 100644 index 0a428d808cd..00000000000 --- a/server/src/test/java/io/druid/server/TestDruidModule.java +++ /dev/null @@ -1,44 +0,0 @@ -/* - * Druid - a distributed column store. - * Copyright (C) 2012, 2013 Metamarkets Group Inc. - * - * This program is free software; you can redistribute it and/or - * modify it under the terms of the GNU General Public License - * as published by the Free Software Foundation; either version 2 - * of the License, or (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program; if not, write to the Free Software - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. - */ - -package io.druid.server; - -import com.fasterxml.jackson.databind.Module; -import com.google.common.collect.ImmutableList; -import com.google.inject.Binder; -import io.druid.initialization.DruidModule; - -import java.util.List; - -/** - */ -public class TestDruidModule implements DruidModule -{ - @Override - public List getJacksonModules() - { - return ImmutableList.of(); - } - - @Override - public void configure(Binder binder) - { - // Do nothing - } -} diff --git a/server/src/test/resources/META-INF/services/io.druid.initialization.DruidModule b/server/src/test/resources/META-INF/services/io.druid.initialization.DruidModule index 7acb2959046..09678a839eb 100644 --- a/server/src/test/resources/META-INF/services/io.druid.initialization.DruidModule +++ b/server/src/test/resources/META-INF/services/io.druid.initialization.DruidModule @@ -1 +1 @@ -io.druid.server.TestDruidModule +io.druid.server.StatusResourceTest$TestDruidModule