This is an automated email from the ASF dual-hosted git repository.
zjffdu pushed a commit to branch branch-0.9
in repository https://gitbox.apache.org/repos/asf/zeppelin.git
The following commit(s) were added to refs/heads/branch-0.9 by this push:
new 8a3ec62 [ZEPPELIN-4833] misleading logging when fail to load plugin
from classpath directly
8a3ec62 is described below
commit 8a3ec62c4adeae9cb94b194cec2c486940130cee
Author: Jeff Zhang <zjf...@apache.org>
AuthorDate: Tue Jun 2 23:00:47 2020 +0800
[ZEPPELIN-4833] misleading logging when fail to load plugin from classpath
directly
### What is this PR for?
Previous plugin loading strategy is to load them from system classloader
first, if fails, then fallback to plugin folder classloader. This would produce
misleading logging which make user think that the plugin is failed to load.
This PR would load plugin from system classloader only when they are builtin
plugins.
### What type of PR is it?
[Improvement]
### Todos
* [ ] - Task
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-4833
### How should this be tested?
* CI pass
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
Author: Jeff Zhang <zjf...@apache.org>
Closes #3784 from zjffdu/ZEPPELIN-4833 and squashes the following commits:
62cd4db59 [Jeff Zhang] [ZEPPELIN-4833] misleading logging when fail to load
plugin from classpath directly
(cherry picked from commit 11780d387ccf7ab86f57313e36fdb5922e66f561)
Signed-off-by: Jeff Zhang <zjf...@apache.org>
---
.../zeppelin/notebook/repo/NotebookRepoSync.java | 6 +-
.../org/apache/zeppelin/plugin/PluginManager.java | 104 ++++++++++++---------
.../repo/NotebookRepoSyncInitializationTest.java | 33 ++++---
.../notebook/repo/NotebookRepoSyncTest.java | 2 +
4 files changed, 83 insertions(+), 62 deletions(-)
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 d679c84..6f37aa2 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 class NotebookRepoSync implements
NotebookRepoWithVersionControl {
// 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 6070c93..056b93e 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.List;
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<String, InterpreterLauncher> cachedLaunchers = new HashMap<>();
+ private List<String> builtinLauncherClassNames = Lists.newArrayList(
+ StandardInterpreterLauncher.class.getName(),
+ SparkInterpreterLauncher.class.getName());
+ private List<String> builtinNotebookRepoClassNames = Lists.newArrayList(
+ VFSNotebookRepo.class.getName(),
+ GitNotebookRepo.class.getName());
+ private List<String> 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 class PluginManager {
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 class PluginManager {
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 @@ public class PluginManager {
}
/**
- * 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 @@ public class PluginManager {
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 class PluginManager {
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 class PluginManager {
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 738d708..655b601 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 java.io.IOException;
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 class NotebookRepoSyncInitializationTest {
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 class NotebookRepoSyncInitializationTest {
}
@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 5dfd5da..31d396e 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 class NotebookRepoSyncTest {
@After
public void tearDown() throws Exception {
delete(ZEPPELIN_HOME);
+ System.clearProperty("zeppelin.isTest");
}
@Test
