diff --git a/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepoSync.java b/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepoSync.java index d679c84cbc0..6f37aa26730 100644 --- a/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepoSync.java +++ b/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepoSync.java @@ -81,10 +81,8 @@ public void init(ZeppelinConfiguration conf) throws IOException { // init the underlying NotebookRepo for (int i = 0; i < Math.min(storageClassNames.length, getMaxRepoNum()); i++) { NotebookRepo notebookRepo = PluginManager.get().loadNotebookRepo(storageClassNames[i].trim()); - if (notebookRepo != null) { - notebookRepo.init(conf); - repos.add(notebookRepo); - } + notebookRepo.init(conf); + repos.add(notebookRepo); } // couldn't initialize any storage, use default diff --git a/zeppelin-zengine/src/main/java/org/apache/zeppelin/plugin/PluginManager.java b/zeppelin-zengine/src/main/java/org/apache/zeppelin/plugin/PluginManager.java index 6070c93ab96..056b93e09a1 100644 --- a/zeppelin-zengine/src/main/java/org/apache/zeppelin/plugin/PluginManager.java +++ b/zeppelin-zengine/src/main/java/org/apache/zeppelin/plugin/PluginManager.java @@ -18,11 +18,18 @@ package org.apache.zeppelin.plugin; import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.Lists; import org.apache.zeppelin.conf.ZeppelinConfiguration; import org.apache.zeppelin.interpreter.launcher.InterpreterLauncher; +import org.apache.zeppelin.interpreter.launcher.SparkInterpreterLauncher; +import org.apache.zeppelin.interpreter.launcher.StandardInterpreterLauncher; import org.apache.zeppelin.interpreter.recovery.RecoveryStorage; +import org.apache.zeppelin.notebook.repo.GitNotebookRepo; import org.apache.zeppelin.notebook.repo.NotebookRepo; +import org.apache.zeppelin.notebook.repo.OldGitNotebookRepo; import org.apache.zeppelin.notebook.repo.OldNotebookRepo; +import org.apache.zeppelin.notebook.repo.OldVFSNotebookRepo; +import org.apache.zeppelin.notebook.repo.VFSNotebookRepo; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -37,7 +44,8 @@ import java.util.Map; /** - * Class for loading Plugins + * Class for loading Plugins. It is singleton and factory class. + * */ public class PluginManager { private static final Logger LOGGER = LoggerFactory.getLogger(PluginManager.class); @@ -49,6 +57,16 @@ public class PluginManager { private Map cachedLaunchers = new HashMap<>(); + private List builtinLauncherClassNames = Lists.newArrayList( + StandardInterpreterLauncher.class.getName(), + SparkInterpreterLauncher.class.getName()); + private List builtinNotebookRepoClassNames = Lists.newArrayList( + VFSNotebookRepo.class.getName(), + GitNotebookRepo.class.getName()); + private List builtinOldNotebookRepoClassNames = Lists.newArrayList( + OldVFSNotebookRepo.class.getName(), + OldGitNotebookRepo.class.getName()); + public static synchronized PluginManager get() { if (instance == null) { instance = new PluginManager(); @@ -58,14 +76,16 @@ public static synchronized PluginManager get() { public NotebookRepo loadNotebookRepo(String notebookRepoClassName) throws IOException { LOGGER.info("Loading NotebookRepo Plugin: " + notebookRepoClassName); - // load plugin from classpath directly first for these builtin NotebookRepo (such as VFSNoteBookRepo - // and GitNotebookRepo). If fails, then try to load it from plugin folder - try { - NotebookRepo notebookRepo = (NotebookRepo) - (Class.forName(notebookRepoClassName).newInstance()); - return notebookRepo; - } catch (InstantiationException | IllegalAccessException | ClassNotFoundException e) { - LOGGER.warn("Fail to instantiate notebookrepo from classpath directly:" + notebookRepoClassName); + if (builtinNotebookRepoClassNames.contains(notebookRepoClassName) || + Boolean.parseBoolean(System.getProperty("zeppelin.isTest", "false"))) { + try { + NotebookRepo notebookRepo = (NotebookRepo) + (Class.forName(notebookRepoClassName).newInstance()); + return notebookRepo; + } catch (InstantiationException | IllegalAccessException | ClassNotFoundException e) { + throw new IOException("Fail to instantiate notebookrepo from classpath directly:" + + notebookRepoClassName, e); + } } String simpleClassName = notebookRepoClassName.substring(notebookRepoClassName.lastIndexOf(".") + 1); @@ -77,13 +97,10 @@ public NotebookRepo loadNotebookRepo(String notebookRepoClassName) throws IOExce try { notebookRepo = (NotebookRepo) (Class.forName(notebookRepoClassName, true, pluginClassLoader)).newInstance(); } catch (InstantiationException | IllegalAccessException | ClassNotFoundException e) { - LOGGER.warn("Fail to instantiate notebookrepo " + notebookRepoClassName + + throw new IOException("Fail to instantiate notebookrepo " + notebookRepoClassName + " from plugin classpath:" + pluginsDir, e); } - if (notebookRepo == null) { - LOGGER.warn("Unable to load NotebookRepo Plugin: " + notebookRepoClassName); - } return notebookRepo; } @@ -93,7 +110,7 @@ private String getOldNotebookRepoClassName(String notebookRepoClassName) { } /** - * This is a temporary class which is used for loading old implemention of NotebookRepo. + * This is a temporary class which is used for loading old implementation of NotebookRepo. * * @param notebookRepoClassName * @return @@ -102,14 +119,16 @@ private String getOldNotebookRepoClassName(String notebookRepoClassName) { public OldNotebookRepo loadOldNotebookRepo(String notebookRepoClassName) throws IOException { String oldNotebookRepoClassName = getOldNotebookRepoClassName(notebookRepoClassName); LOGGER.info("Loading OldNotebookRepo Plugin: " + oldNotebookRepoClassName); - // load plugin from classpath directly first for these builtin NotebookRepo (such as VFSNoteBookRepo - // and GitNotebookRepo). If fails, then try to load it from plugin folder - try { - OldNotebookRepo notebookRepo = (OldNotebookRepo) - (Class.forName(oldNotebookRepoClassName).newInstance()); - return notebookRepo; - } catch (InstantiationException | IllegalAccessException | ClassNotFoundException e) { - LOGGER.warn("Fail to instantiate notebookrepo from classpath directly:" + oldNotebookRepoClassName); + if (builtinOldNotebookRepoClassNames.contains(oldNotebookRepoClassName) || + Boolean.parseBoolean(System.getProperty("zeppelin.isTest", "false"))) { + try { + OldNotebookRepo notebookRepo = (OldNotebookRepo) + (Class.forName(oldNotebookRepoClassName).newInstance()); + return notebookRepo; + } catch (InstantiationException | IllegalAccessException | ClassNotFoundException e) { + throw new IOException("Fail to instantiate notebookrepo from classpath directly:" + + oldNotebookRepoClassName); + } } String simpleClassName = notebookRepoClassName.substring(notebookRepoClassName.lastIndexOf(".") + 1); @@ -121,13 +140,10 @@ public OldNotebookRepo loadOldNotebookRepo(String notebookRepoClassName) throws try { notebookRepo = (OldNotebookRepo) (Class.forName(oldNotebookRepoClassName, true, pluginClassLoader)).newInstance(); } catch (InstantiationException | IllegalAccessException | ClassNotFoundException e) { - LOGGER.warn("Fail to instantiate notebookrepo " + oldNotebookRepoClassName + + throw new IOException("Fail to instantiate notebookrepo " + oldNotebookRepoClassName + " from plugin classpath:" + pluginsDir, e); } - if (notebookRepo == null) { - LOGGER.warn("Unable to load NotebookRepo Plugin: " + oldNotebookRepoClassName); - } return notebookRepo; } @@ -138,36 +154,36 @@ public synchronized InterpreterLauncher loadInterpreterLauncher(String launcherP if (cachedLaunchers.containsKey(launcherPlugin)) { return cachedLaunchers.get(launcherPlugin); } - LOGGER.info("Loading Interpreter Launcher Plugin: " + launcherPlugin); - // load plugin from classpath directly first for these builtin InterpreterLauncher. - // If fails, then try to load it from plugin folder. - try { - InterpreterLauncher launcher = (InterpreterLauncher) - (Class.forName("org.apache.zeppelin.interpreter.launcher." + launcherPlugin)) - .getConstructor(ZeppelinConfiguration.class, RecoveryStorage.class) - .newInstance(zConf, recoveryStorage); - return launcher; - } catch (InstantiationException | IllegalAccessException | ClassNotFoundException - | NoSuchMethodException | InvocationTargetException e) { - LOGGER.warn("Fail to instantiate InterpreterLauncher from classpath directly:" + launcherPlugin, e); + String launcherClassName = "org.apache.zeppelin.interpreter.launcher." + launcherPlugin; + LOGGER.info("Loading Interpreter Launcher Plugin: " + launcherClassName); + + if (builtinLauncherClassNames.contains(launcherClassName) || + Boolean.parseBoolean(System.getProperty("zeppelin.isTest", "false"))) { + try { + InterpreterLauncher launcher = (InterpreterLauncher) + (Class.forName(launcherClassName)) + .getConstructor(ZeppelinConfiguration.class, RecoveryStorage.class) + .newInstance(zConf, recoveryStorage); + return launcher; + } catch (InstantiationException | IllegalAccessException | ClassNotFoundException + | NoSuchMethodException | InvocationTargetException e) { + throw new IOException("Fail to instantiate InterpreterLauncher from classpath directly:" + + launcherClassName, e); + } } URLClassLoader pluginClassLoader = getPluginClassLoader(pluginsDir, "Launcher", launcherPlugin); - String pluginClass = "org.apache.zeppelin.interpreter.launcher." + launcherPlugin; InterpreterLauncher launcher = null; try { - launcher = (InterpreterLauncher) (Class.forName(pluginClass, true, pluginClassLoader)) + launcher = (InterpreterLauncher) (Class.forName(launcherClassName, true, pluginClassLoader)) .getConstructor(ZeppelinConfiguration.class, RecoveryStorage.class) .newInstance(zConf, recoveryStorage); } catch (InstantiationException | IllegalAccessException | ClassNotFoundException | NoSuchMethodException | InvocationTargetException e) { - LOGGER.warn("Fail to instantiate Launcher " + launcherPlugin + + throw new IOException("Fail to instantiate Launcher " + launcherPlugin + " from plugin pluginDir: " + pluginsDir, e); } - if (launcher == null) { - throw new IOException("Fail to load plugin: " + launcherPlugin); - } cachedLaunchers.put(launcherPlugin, launcher); return launcher; } diff --git a/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoSyncInitializationTest.java b/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoSyncInitializationTest.java index 738d708d19a..655b601fafd 100644 --- a/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoSyncInitializationTest.java +++ b/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoSyncInitializationTest.java @@ -31,10 +31,11 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; //TODO(zjffdu) move it to zeppelin-zengine public class NotebookRepoSyncInitializationTest { - private static final Logger LOG = LoggerFactory.getLogger(NotebookRepoSyncInitializationTest.class); + private static final Logger LOGGER = LoggerFactory.getLogger(NotebookRepoSyncInitializationTest.class); private String validFirstStorageClass = "org.apache.zeppelin.notebook.repo.VFSNotebookRepo"; private String validSecondStorageClass = "org.apache.zeppelin.notebook.repo.mock.VFSNotebookRepoMock"; private String invalidStorageClass = "org.apache.zeppelin.notebook.repo.DummyNotebookRepo"; @@ -47,12 +48,12 @@ public class NotebookRepoSyncInitializationTest { @Before public void setUp(){ System.setProperty(ConfVars.ZEPPELIN_PLUGINS_DIR.getVarName(), new File("../../../plugins").getAbsolutePath()); - //setup routine + System.setProperty("zeppelin.isTest", "true"); } @After public void tearDown() { - //tear-down routine + System.clearProperty("zeppelin.isTest"); } @Test @@ -101,11 +102,13 @@ public void invalidInitTwoStorageTest() throws IOException { System.setProperty(ConfVars.ZEPPELIN_NOTEBOOK_STORAGE.getVarName(), invalidTwoStorageConf); ZeppelinConfiguration conf = ZeppelinConfiguration.create(); // create repo - NotebookRepoSync notebookRepoSync = new NotebookRepoSync(conf); - // check that second didn't initialize - LOG.info(" " + notebookRepoSync.getRepoCount()); - assertEquals(notebookRepoSync.getRepoCount(), 1); - assertTrue(notebookRepoSync.getRepo(0) instanceof VFSNotebookRepo); + try { + NotebookRepoSync notebookRepoSync = new NotebookRepoSync(conf); + fail("Should throw exception due to invalid NotebookRepo"); + } catch (IOException e) { + LOGGER.error(e.getMessage()); + assertTrue(e.getMessage().contains("Fail to instantiate notebookrepo from classpath directly")); + } } @Test @@ -148,14 +151,16 @@ public void initEmptyStorageTest() throws IOException { } @Test - public void initOneDummyStorageTest() throws IOException { - // set confs + public void initOneDummyStorageTest() { System.setProperty(ConfVars.ZEPPELIN_NOTEBOOK_STORAGE.getVarName(), invalidStorageClass); ZeppelinConfiguration conf = ZeppelinConfiguration.create(); // create repo - NotebookRepoSync notebookRepoSync = new NotebookRepoSync(conf); - // check initialization of one default storage instead of invalid one - assertEquals(notebookRepoSync.getRepoCount(), 1); - assertTrue(notebookRepoSync.getRepo(0) instanceof NotebookRepo); + try { + NotebookRepoSync notebookRepoSync = new NotebookRepoSync(conf); + fail("Should throw exception due to invalid NotebookRepo"); + } catch (IOException e) { + LOGGER.error(e.getMessage()); + assertTrue(e.getMessage().contains("Fail to instantiate notebookrepo from classpath directly")); + } } } \ No newline at end of file diff --git a/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoSyncTest.java b/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoSyncTest.java index 5dfd5da7d71..31d396e0d2e 100644 --- a/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoSyncTest.java +++ b/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoSyncTest.java @@ -72,6 +72,7 @@ public class NotebookRepoSyncTest { @Before public void setUp() throws Exception { + System.setProperty("zeppelin.isTest", "true"); ZEPPELIN_HOME = Files.createTempDir(); new File(ZEPPELIN_HOME, "conf").mkdirs(); String mainNotePath = ZEPPELIN_HOME.getAbsolutePath() + "/notebook"; @@ -110,6 +111,7 @@ public void setUp() throws Exception { @After public void tearDown() throws Exception { delete(ZEPPELIN_HOME); + System.clearProperty("zeppelin.isTest"); } @Test