You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2020/04/06 09:59:38 UTC
[GitHub] [incubator-doris] morningman opened a new pull request #3267: [Bug]
Fix some bugs of install/uninstall plugins
morningman opened a new pull request #3267: [Bug] Fix some bugs of install/uninstall plugins
URL: https://github.com/apache/incubator-doris/pull/3267
1. Avoid losing plugin if plugin failed to load when replaying
When in replay process, the plugin should always be added to the plugin manager,
even if that plugin failed to be loaded.
2. `show plugin` statement should show all plugins, not only the successfully installed plugins.
3. plugin's name should be unique globally and case insensitive.
4. Avoid creating new instances of plugins when doing metadata checkpoint.
5. Add a __builtin_ prefix for builtin plugins.
ISSUE: #3266
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] Seaven commented on a change in pull request
#3267: [Bug] Fix some bugs of install/uninstall plugins
Posted by GitBox <gi...@apache.org>.
Seaven commented on a change in pull request #3267: [Bug] Fix some bugs of install/uninstall plugins
URL: https://github.com/apache/incubator-doris/pull/3267#discussion_r405924531
##########
File path: fe/src/main/java/org/apache/doris/plugin/PluginMgr.java
##########
@@ -86,25 +112,26 @@ public PluginInfo installPlugin(InstallPluginStmt stmt) throws IOException, User
try {
PluginInfo info = pluginLoader.getPluginInfo();
-
- if (plugins[info.getTypeId()].containsKey(info.getName())) {
+
+ if (checkDynamicPluginNameExist(info.getName())) {
throw new UserException("plugin " + info.getName() + " has already been installed.");
}
// install plugin
pluginLoader.install();
pluginLoader.setStatus(PluginStatus.INSTALLED);
- if (plugins[info.getTypeId()].putIfAbsent(info.getName(), pluginLoader) != null) {
- pluginLoader.uninstall();
+ if (!addDynamicPluginNameIfAbsent(info.getName())) {
throw new UserException("plugin " + info.getName() + " has already been installed.");
}
-
+
+ plugins[info.getTypeId()].put(info.getName(), pluginLoader);
+
Catalog.getCurrentCatalog().getEditLog().logInstallPlugin(info);
- LOG.info("install plugin = " + info.getName());
+ LOG.info("install plugin {}", info.getName());
return info;
} catch (IOException | UserException e) {
- pluginLoader.setStatus(PluginStatus.ERROR);
+ pluginLoader.uninstall();
Review comment:
Ignore, installPath will not exist if it is not installed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] Seaven commented on a change in pull request
#3267: [Bug] Fix some bugs of install/uninstall plugins
Posted by GitBox <gi...@apache.org>.
Seaven commented on a change in pull request #3267: [Bug] Fix some bugs of install/uninstall plugins
URL: https://github.com/apache/incubator-doris/pull/3267#discussion_r404535321
##########
File path: fe/src/main/java/org/apache/doris/plugin/PluginMgr.java
##########
@@ -86,25 +112,26 @@ public PluginInfo installPlugin(InstallPluginStmt stmt) throws IOException, User
try {
PluginInfo info = pluginLoader.getPluginInfo();
-
- if (plugins[info.getTypeId()].containsKey(info.getName())) {
+
+ if (checkDynamicPluginNameExist(info.getName())) {
throw new UserException("plugin " + info.getName() + " has already been installed.");
}
// install plugin
pluginLoader.install();
pluginLoader.setStatus(PluginStatus.INSTALLED);
- if (plugins[info.getTypeId()].putIfAbsent(info.getName(), pluginLoader) != null) {
- pluginLoader.uninstall();
+ if (!addDynamicPluginNameIfAbsent(info.getName())) {
throw new UserException("plugin " + info.getName() + " has already been installed.");
}
-
+
+ plugins[info.getTypeId()].put(info.getName(), pluginLoader);
+
Catalog.getCurrentCatalog().getEditLog().logInstallPlugin(info);
- LOG.info("install plugin = " + info.getName());
+ LOG.info("install plugin {}", info.getName());
return info;
} catch (IOException | UserException e) {
- pluginLoader.setStatus(PluginStatus.ERROR);
+ pluginLoader.uninstall();
Review comment:
Can't uninstall in there, will cause uninstall and delete real plugin if install same plugin
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] Seaven commented on a change in pull request
#3267: [Bug] Fix some bugs of install/uninstall plugins
Posted by GitBox <gi...@apache.org>.
Seaven commented on a change in pull request #3267: [Bug] Fix some bugs of install/uninstall plugins
URL: https://github.com/apache/incubator-doris/pull/3267#discussion_r404535321
##########
File path: fe/src/main/java/org/apache/doris/plugin/PluginMgr.java
##########
@@ -86,25 +112,26 @@ public PluginInfo installPlugin(InstallPluginStmt stmt) throws IOException, User
try {
PluginInfo info = pluginLoader.getPluginInfo();
-
- if (plugins[info.getTypeId()].containsKey(info.getName())) {
+
+ if (checkDynamicPluginNameExist(info.getName())) {
throw new UserException("plugin " + info.getName() + " has already been installed.");
}
// install plugin
pluginLoader.install();
pluginLoader.setStatus(PluginStatus.INSTALLED);
- if (plugins[info.getTypeId()].putIfAbsent(info.getName(), pluginLoader) != null) {
- pluginLoader.uninstall();
+ if (!addDynamicPluginNameIfAbsent(info.getName())) {
throw new UserException("plugin " + info.getName() + " has already been installed.");
}
-
+
+ plugins[info.getTypeId()].put(info.getName(), pluginLoader);
+
Catalog.getCurrentCatalog().getEditLog().logInstallPlugin(info);
- LOG.info("install plugin = " + info.getName());
+ LOG.info("install plugin {}", info.getName());
return info;
} catch (IOException | UserException e) {
- pluginLoader.setStatus(PluginStatus.ERROR);
+ pluginLoader.uninstall();
Review comment:
Can't uninstall in there, will cause uninstall real plugin
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] morningman commented on a change in pull request
#3267: [Bug] Fix some bugs of install/uninstall plugins
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #3267: [Bug] Fix some bugs of install/uninstall plugins
URL: https://github.com/apache/incubator-doris/pull/3267#discussion_r405517112
##########
File path: fe/src/main/java/org/apache/doris/plugin/PluginMgr.java
##########
@@ -41,17 +43,23 @@
import java.util.List;
import java.util.Map;
import java.util.Objects;
+import java.util.Set;
public class PluginMgr implements Writable {
private final static Logger LOG = LogManager.getLogger(PluginMgr.class);
+ public final static String BUILTIN_PLUGIN_PREFIX = "__builtin_";
+
private final Map<String, PluginLoader>[] plugins;
+ // all dynamic plugins should have unique names,
+ private final Set<String> dynamicPluginNames;
public PluginMgr() {
- plugins = new Map[PluginType.MAX_PLUGIN_SIZE];
- for (int i = 0; i < PluginType.MAX_PLUGIN_SIZE; i++) {
- plugins[i] = Maps.newConcurrentMap();
+ plugins = new Map[PluginType.MAX_PLUGIN_TYPE_SIZE];
+ for (int i = 0; i < PluginType.MAX_PLUGIN_TYPE_SIZE; i++) {
+ plugins[i] = Maps.newTreeMap(String.CASE_INSENSITIVE_ORDER);
}
+ dynamicPluginNames = Sets.newTreeSet(String.CASE_INSENSITIVE_ORDER);
Review comment:
No, Concurrent collection only solves the problem of concurrency conflicts, but what I need here is a `serializable isolation`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] Seaven commented on a change in pull request
#3267: [Bug] Fix some bugs of install/uninstall plugins
Posted by GitBox <gi...@apache.org>.
Seaven commented on a change in pull request #3267: [Bug] Fix some bugs of install/uninstall plugins
URL: https://github.com/apache/incubator-doris/pull/3267#discussion_r404532213
##########
File path: fe/src/main/java/org/apache/doris/plugin/PluginMgr.java
##########
@@ -41,17 +43,23 @@
import java.util.List;
import java.util.Map;
import java.util.Objects;
+import java.util.Set;
public class PluginMgr implements Writable {
private final static Logger LOG = LogManager.getLogger(PluginMgr.class);
+ public final static String BUILTIN_PLUGIN_PREFIX = "__builtin_";
+
private final Map<String, PluginLoader>[] plugins;
+ // all dynamic plugins should have unique names,
+ private final Set<String> dynamicPluginNames;
public PluginMgr() {
- plugins = new Map[PluginType.MAX_PLUGIN_SIZE];
- for (int i = 0; i < PluginType.MAX_PLUGIN_SIZE; i++) {
- plugins[i] = Maps.newConcurrentMap();
+ plugins = new Map[PluginType.MAX_PLUGIN_TYPE_SIZE];
+ for (int i = 0; i < PluginType.MAX_PLUGIN_TYPE_SIZE; i++) {
+ plugins[i] = Maps.newTreeMap(String.CASE_INSENSITIVE_ORDER);
}
+ dynamicPluginNames = Sets.newTreeSet(String.CASE_INSENSITIVE_ORDER);
Review comment:
Maybe you need ConcurrentSkipListSet? I guess
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] morningman commented on a change in pull request
#3267: [Bug] Fix some bugs of install/uninstall plugins
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #3267: [Bug] Fix some bugs of install/uninstall plugins
URL: https://github.com/apache/incubator-doris/pull/3267#discussion_r405518828
##########
File path: fe/src/main/java/org/apache/doris/plugin/PluginMgr.java
##########
@@ -86,25 +112,26 @@ public PluginInfo installPlugin(InstallPluginStmt stmt) throws IOException, User
try {
PluginInfo info = pluginLoader.getPluginInfo();
-
- if (plugins[info.getTypeId()].containsKey(info.getName())) {
+
+ if (checkDynamicPluginNameExist(info.getName())) {
Review comment:
I think it works.
I first using `checkDynamicPluginNameExist` to check if name already exist.
And after installing, I use `addDynamicPluginNameIfAbsent()` to `check-and-add` name to the `dynamicPluginNames` set. `check-and-add` is an atomic operation.
And `plugins[]` array is just for saving the plugin, I do not use it to check name existence.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] Seaven commented on a change in pull request
#3267: [Bug] Fix some bugs of install/uninstall plugins
Posted by GitBox <gi...@apache.org>.
Seaven commented on a change in pull request #3267: [Bug] Fix some bugs of install/uninstall plugins
URL: https://github.com/apache/incubator-doris/pull/3267#discussion_r404534865
##########
File path: fe/src/main/java/org/apache/doris/plugin/PluginMgr.java
##########
@@ -86,25 +112,26 @@ public PluginInfo installPlugin(InstallPluginStmt stmt) throws IOException, User
try {
PluginInfo info = pluginLoader.getPluginInfo();
-
- if (plugins[info.getTypeId()].containsKey(info.getName())) {
+
+ if (checkDynamicPluginNameExist(info.getName())) {
Review comment:
Using two containers does not avoid concurrency problems
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] Seaven commented on a change in pull request
#3267: [Bug] Fix some bugs of install/uninstall plugins
Posted by GitBox <gi...@apache.org>.
Seaven commented on a change in pull request #3267: [Bug] Fix some bugs of install/uninstall plugins
URL: https://github.com/apache/incubator-doris/pull/3267#discussion_r404537885
##########
File path: fe/src/main/java/org/apache/doris/plugin/PluginMgr.java
##########
@@ -113,30 +140,47 @@ public PluginInfo installPlugin(InstallPluginStmt stmt) throws IOException, User
* Dynamic uninstall plugin
*/
public PluginInfo uninstallPlugin(String name) throws IOException, UserException {
- for (int i = 0; i < PluginType.MAX_PLUGIN_SIZE; i++) {
+ if (!checkDynamicPluginNameExist(name)) {
+ throw new DdlException("Plugin " + name + " does not exist");
+ }
+
+ for (int i = 0; i < PluginType.MAX_PLUGIN_TYPE_SIZE; i++) {
if (plugins[i].containsKey(name)) {
PluginLoader loader = plugins[i].get(name);
+ if (loader == null) {
+ // this is not a atomic operation, so even if containsKey() is true,
+ // we may still get null object by get() method
+ continue;
+ }
- if (null != loader && loader.isDynamicPlugin()) {
- loader.pluginUninstallValid();
- loader.setStatus(PluginStatus.UNINSTALLING);
- // uninstall plugin
- loader.uninstall();
- plugins[i].remove(name);
-
- loader.setStatus(PluginStatus.UNINSTALLED);
- return loader.getPluginInfo();
+ if (!loader.isDynamicPlugin()) {
+ throw new DdlException("Only support uninstall dynamic plugins");
}
+
+ loader.pluginUninstallValid();
+ loader.setStatus(PluginStatus.UNINSTALLING);
+ // uninstall plugin
+ loader.uninstall();
+ plugins[i].remove(name);
+ loader.setStatus(PluginStatus.UNINSTALLED);
+ removeDynamicPluginName(name);
+
+ // do not get plugin info by calling loader.getPluginInfo(). That method will try to
+ // reload the plugin properties from source if this plugin is not installed successfully.
+ // Here we only need the plugin's name for persisting.
+ // TODO(cmy): This is a bad design to couple the persist info with PluginInfo, but for
+ // the compatibility, I till use this method.
Review comment:
It is a good idea not to use PluginInfo for persist
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] morningman commented on a change in pull request
#3267: [Bug] Fix some bugs of install/uninstall plugins
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #3267: [Bug] Fix some bugs of install/uninstall plugins
URL: https://github.com/apache/incubator-doris/pull/3267#discussion_r405519972
##########
File path: fe/src/main/java/org/apache/doris/plugin/PluginMgr.java
##########
@@ -113,30 +140,47 @@ public PluginInfo installPlugin(InstallPluginStmt stmt) throws IOException, User
* Dynamic uninstall plugin
*/
public PluginInfo uninstallPlugin(String name) throws IOException, UserException {
- for (int i = 0; i < PluginType.MAX_PLUGIN_SIZE; i++) {
+ if (!checkDynamicPluginNameExist(name)) {
+ throw new DdlException("Plugin " + name + " does not exist");
+ }
+
+ for (int i = 0; i < PluginType.MAX_PLUGIN_TYPE_SIZE; i++) {
if (plugins[i].containsKey(name)) {
PluginLoader loader = plugins[i].get(name);
+ if (loader == null) {
+ // this is not a atomic operation, so even if containsKey() is true,
+ // we may still get null object by get() method
+ continue;
+ }
- if (null != loader && loader.isDynamicPlugin()) {
- loader.pluginUninstallValid();
- loader.setStatus(PluginStatus.UNINSTALLING);
- // uninstall plugin
- loader.uninstall();
- plugins[i].remove(name);
-
- loader.setStatus(PluginStatus.UNINSTALLED);
- return loader.getPluginInfo();
+ if (!loader.isDynamicPlugin()) {
+ throw new DdlException("Only support uninstall dynamic plugins");
}
+
+ loader.pluginUninstallValid();
+ loader.setStatus(PluginStatus.UNINSTALLING);
+ // uninstall plugin
+ loader.uninstall();
+ plugins[i].remove(name);
+ loader.setStatus(PluginStatus.UNINSTALLED);
+ removeDynamicPluginName(name);
+
+ // do not get plugin info by calling loader.getPluginInfo(). That method will try to
+ // reload the plugin properties from source if this plugin is not installed successfully.
+ // Here we only need the plugin's name for persisting.
+ // TODO(cmy): This is a bad design to couple the persist info with PluginInfo, but for
+ // the compatibility, I till use this method.
Review comment:
Yes, but I don't want to change the meta persistence method in this pull request. Maybe changed later.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] morningman merged pull request #3267: [Bug] Fix
some bugs of install/uninstall plugins
Posted by GitBox <gi...@apache.org>.
morningman merged pull request #3267: [Bug] Fix some bugs of install/uninstall plugins
URL: https://github.com/apache/incubator-doris/pull/3267
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] Seaven commented on a change in pull request
#3267: [Bug] Fix some bugs of install/uninstall plugins
Posted by GitBox <gi...@apache.org>.
Seaven commented on a change in pull request #3267: [Bug] Fix some bugs of install/uninstall plugins
URL: https://github.com/apache/incubator-doris/pull/3267#discussion_r404535321
##########
File path: fe/src/main/java/org/apache/doris/plugin/PluginMgr.java
##########
@@ -86,25 +112,26 @@ public PluginInfo installPlugin(InstallPluginStmt stmt) throws IOException, User
try {
PluginInfo info = pluginLoader.getPluginInfo();
-
- if (plugins[info.getTypeId()].containsKey(info.getName())) {
+
+ if (checkDynamicPluginNameExist(info.getName())) {
throw new UserException("plugin " + info.getName() + " has already been installed.");
}
// install plugin
pluginLoader.install();
pluginLoader.setStatus(PluginStatus.INSTALLED);
- if (plugins[info.getTypeId()].putIfAbsent(info.getName(), pluginLoader) != null) {
- pluginLoader.uninstall();
+ if (!addDynamicPluginNameIfAbsent(info.getName())) {
throw new UserException("plugin " + info.getName() + " has already been installed.");
}
-
+
+ plugins[info.getTypeId()].put(info.getName(), pluginLoader);
+
Catalog.getCurrentCatalog().getEditLog().logInstallPlugin(info);
- LOG.info("install plugin = " + info.getName());
+ LOG.info("install plugin {}", info.getName());
return info;
} catch (IOException | UserException e) {
- pluginLoader.setStatus(PluginStatus.ERROR);
+ pluginLoader.uninstall();
Review comment:
Can't uninstall in there, will cause uninstall and delete real plugin
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org