[GitHub] sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins Handler
sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins
Handler
URL: https://github.com/apache/drill/pull/1345#discussion_r199314863
##
File path:
contrib/storage-kafka/src/main/resources/bootstrap-storage-plugins.json
##
@@ -2,8 +2,8 @@
"storage":{
kafka : {
type:"kafka",
- enabled: false,
- kafkaConsumerProps: {"bootstrap.servers":"localhost:9092", "group.id" :
"drill-consumer"}
+ kafkaConsumerProps: {"bootstrap.servers":"localhost:9092", "group.id" :
"drill-consumer"},
+ enabled: false
Review comment:
Hmm do you mean on WebUI because I am seeing it in the beginning only.
Attaching the snapshot
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins Handler
sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins
Handler
URL: https://github.com/apache/drill/pull/1345#discussion_r199315728
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginsHandler.java
##
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store;
+
+import org.apache.drill.common.logical.StoragePluginConfig;
+import org.apache.drill.exec.planner.logical.StoragePlugins;
+import org.apache.drill.exec.store.sys.PersistentStore;
+
+
+/**
+ * Storage plugins handler is an additional service for updating storage
plugins configs from the file
+ */
+public interface StoragePluginsHandler {
+
+ /**
+ * Update incoming storage plugins configs from persistence store if
present, otherwise bootstrap plugins configs.
+ * One of the params should be null, second shouldn't
+ *
+ * @param persistentStore the last storage plugins configs from persistence
store
+ * @param bootstrapPlugins bootstrap storage plugins, which are used in case
of first Drill start up
+ * @return all storage plugins, which should be loaded into persistence store
+ */
+ void loadPlugins(PersistentStore persistentStore,
StoragePlugins bootstrapPlugins);
Review comment:
- One is not true since with current approach, let say if there are N
plugins in store.
1) Caller of `loadPlugins` will get all the plugins from `persistentStore`
and assign `bootstrapPlugins` to null or non-null value. There will be N calls
internally to PStore.
2) Then inside loadPlugins if there is newPluginConfig present it will
iterate for each plugin config, get that plugin config from persistent store
and then write the update plugin config to the store. For each plugin there is
get and put call so total 2N calls here.
So overall there are 3N calls to the PStore.
Whereas if caller is directly passing list of plugins whether it be built
using bootstrap or persistentStore, it makes N call for that. And later when it
gets back updated plugin from loadPlugins method then it writes those one by
one to the PStore which is again N calls to PStore hence overall there are 2N
calls.
- My personal opinion is registry should only use the store rather than this
handler since registry creates it as well. And that way all read and write to
store is also happening at once place (i.e. in registry). I will leave the
choice up to you.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins Handler
sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins
Handler
URL: https://github.com/apache/drill/pull/1345#discussion_r199315205
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginsHandlerService.java
##
@@ -0,0 +1,130 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.base.Charsets;
+import com.google.common.io.Resources;
+import com.jasonclawson.jackson.dataformat.hocon.HoconFactory;
+import org.apache.drill.common.config.CommonConstants;
+import org.apache.drill.common.config.LogicalPlanPersistence;
+import org.apache.drill.common.exceptions.DrillRuntimeException;
+import org.apache.drill.common.logical.StoragePluginConfig;
+import org.apache.drill.common.scanner.ClassPathScanner;
+import org.apache.drill.exec.planner.logical.StoragePlugins;
+import org.apache.drill.exec.server.DrillbitContext;
+import org.apache.drill.exec.store.sys.PersistentStore;
+
+import javax.annotation.Nullable;
+import javax.validation.constraints.NotNull;
+import java.io.IOException;
+import java.net.URL;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+
+/**
+ * Drill plugins handler, which allows to update storage plugins configs from
the
+ * {@link CommonConstants#STORAGE_PLUGINS_OVERRIDE_CONF} conf file
+ *
+ * TODO: DRILL-6564: It can be improved with configs versioning and service of
creating
+ * {@link CommonConstants#STORAGE_PLUGINS_OVERRIDE_CONF}
+ */
+public class StoragePluginsHandlerService implements StoragePluginsHandler {
+ private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(StoragePluginsHandlerService.class);
+
+ private final LogicalPlanPersistence hoconLogicalPlanPersistence;
+
+ public StoragePluginsHandlerService(DrillbitContext context) {
+hoconLogicalPlanPersistence = new
LogicalPlanPersistence(context.getConfig(), context.getClasspathScan(),
+new ObjectMapper(new HoconFactory()));
+ }
+
+ @Override
+ public void loadPlugins(@NotNull PersistentStore
persistentStore,
+ @Nullable StoragePlugins bootstrapPlugins) {
+// if bootstrapPlugins is not null -- fresh Drill set up
+StoragePlugins pluginsToBeWrittenToPersistentStore;
+
+StoragePlugins newPlugins = getNewStoragePlugins();
+
+if (newPlugins != null) {
+ pluginsToBeWrittenToPersistentStore = new StoragePlugins(new
HashMap<>());
+ Optional.ofNullable(bootstrapPlugins)
+ .ifPresent(pluginsToBeWrittenToPersistentStore::putAll);
+
+ for (Map.Entry newPlugin : newPlugins) {
+String pluginName = newPlugin.getKey();
+StoragePluginConfig oldPluginConfig =
Optional.ofNullable(bootstrapPlugins)
+.map(plugins -> plugins.getConfig(pluginName))
+.orElse(persistentStore.get(pluginName));
+StoragePluginConfig updatedStatusPluginConfig =
updatePluginStatus(oldPluginConfig, newPlugin.getValue());
+pluginsToBeWrittenToPersistentStore.put(pluginName,
updatedStatusPluginConfig);
+ }
+} else {
+ pluginsToBeWrittenToPersistentStore = bootstrapPlugins;
+}
+
+// load pluginsToBeWrittenToPersistentStore to Persistent Store
+Optional.ofNullable(pluginsToBeWrittenToPersistentStore)
+.ifPresent(plugins -> plugins.forEach(plugin ->
persistentStore.put(plugin.getKey(), plugin.getValue(;
+ }
+
+ /**
+ * Helper method to identify the enabled status for new storage plugins
config. If this status is absent in the updater
+ * file, the status is kept from the configs, which are going to be updated
+ *
+ * @param oldPluginConfig current storage plugin config from Persistent
Store or bootstrap config file
+ * @param newPluginConfig new storage plugin config
+ * @return new storage plugin config with updated enabled status
+ */
+ private StoragePluginConfig updatePluginStatus(@Nullable StoragePluginConfig
oldPluginConfig,
+ @NotNull StoragePluginConfig
newPluginConfig) {
+if
[GitHub] sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins Handler
sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins
Handler
URL: https://github.com/apache/drill/pull/1345#discussion_r199306981
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistryImpl.java
##
@@ -121,69 +121,73 @@ public StoragePlugin load(StoragePluginConfig config)
throws Exception {
}
@Override
- public void init() throws DrillbitStartupException {
+ public void init() {
availablePlugins = findAvailablePlugins(classpathScan);
+try {
+ StoragePlugins bootstrapPlugins = pluginSystemTable.getAll().hasNext() ?
null : loadBootstrapPlugins();
-// create registered plugins defined in "storage-plugins.json"
-plugins.putAll(createPlugins());
+ StoragePluginsHandler storagePluginsUpdaterService = new
StoragePluginsHandlerService(context);
+ storagePluginsUpdaterService.loadPlugins(pluginSystemTable,
bootstrapPlugins);
+
+ // Defines enabled plugins
+ defineEnabledPlugins();
+} catch (IOException e) {
+ logger.error("Failure setting up storage enabledPlugins. Drillbit
exiting.", e);
+ throw new IllegalStateException(e);
+}
}
- @SuppressWarnings("resource")
- private Map createPlugins() throws
DrillbitStartupException {
-try {
- /*
- * Check if the storage plugins system table has any entries. If not,
load the boostrap-storage-plugin file into
- * the system table.
- */
- if (!pluginSystemTable.getAll().hasNext()) {
-// bootstrap load the config since no plugins are stored.
-logger.info("No storage plugin instances configured in persistent
store, loading bootstrap configuration.");
-Collection urls =
ClassPathScanner.forResource(ExecConstants.BOOTSTRAP_STORAGE_PLUGINS_FILE,
false);
-if (urls != null && !urls.isEmpty()) {
- logger.info("Loading the storage plugin configs from URLs {}.",
urls);
- Map pluginURLMap = Maps.newHashMap();
- for (URL url : urls) {
-String pluginsData = Resources.toString(url, Charsets.UTF_8);
-StoragePlugins plugins =
lpPersistence.getMapper().readValue(pluginsData, StoragePlugins.class);
-for (Map.Entry config : plugins) {
- if (!definePluginConfig(config.getKey(), config.getValue())) {
-logger.warn("Duplicate plugin instance '{}' defined in [{},
{}], ignoring the later one.",
-config.getKey(), pluginURLMap.get(config.getKey()), url);
-continue;
- }
- pluginURLMap.put(config.getKey(), url);
-}
- }
-} else {
- throw new IOException("Failure finding " +
ExecConstants.BOOTSTRAP_STORAGE_PLUGINS_FILE);
-}
+ /**
+ * Read bootstrap storage plugins {@link
ExecConstants#BOOTSTRAP_STORAGE_PLUGINS_FILE} files for the first fresh
+ * instantiating of Drill
+ *
+ * @return bootstrap storage plugins
+ * @throws IOException if a read error occurs
+ */
+ private StoragePlugins loadBootstrapPlugins() throws IOException {
+// bootstrap load the config since no plugins are stored.
+logger.info("No storage plugin instances configured in persistent store,
loading bootstrap configuration.");
+Set urls =
ClassPathScanner.forResource(ExecConstants.BOOTSTRAP_STORAGE_PLUGINS_FILE,
false);
+if (urls != null && !urls.isEmpty()) {
+ logger.info("Loading the storage plugin configs from URLs {}.", urls);
+ StoragePlugins bootstrapPlugins = new StoragePlugins(new HashMap<>());
+ for (URL url : urls) {
+String pluginsData = Resources.toString(url, Charsets.UTF_8);
+
bootstrapPlugins.putAll(lpPersistence.getMapper().readValue(pluginsData,
StoragePlugins.class));
Review comment:
This behavior is different from existing one since here in case of duplicate
plugin config we will choose the last one whereas previously we were logging a
warning and considering first seen plugin.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins Handler
sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins
Handler
URL: https://github.com/apache/drill/pull/1345#discussion_r199218902
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginsHandler.java
##
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store;
+
+import org.apache.drill.common.logical.StoragePluginConfig;
+import org.apache.drill.exec.planner.logical.StoragePlugins;
+import org.apache.drill.exec.store.sys.PersistentStore;
+
+
+/**
+ * Storage plugins handler is an additional service for updating storage
plugins configs from the file
+ */
+public interface StoragePluginsHandler {
+
+ /**
+ * Update incoming storage plugins configs from persistence store if
present, otherwise bootstrap plugins configs.
+ * One of the params should be null, second shouldn't
+ *
+ * @param bootstrapPlugins bootstrap storage plugins, which are used in case
of first Drill start up
+ * @param persistentStore the last storage plugins configs from persistence
store
+ * @return all storage plugins, which should be loaded into persistence store
+ */
+ StoragePlugins updatePlugins(StoragePlugins bootstrapPlugins,
PersistentStore persistentStore);
Review comment:
Also I think the signature of the function should be to accept only
`StoragePlugins` now it's upto caller to call it with bootstrap configuration
or one inside PersistentStore. Basically the handler with get a initial
configuration and it will produce a final configuration using that initial
config and it's own logic.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins Handler
sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins
Handler
URL: https://github.com/apache/drill/pull/1345#discussion_r199305835
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/NamedStoragePluginConfig.java
##
@@ -17,22 +17,51 @@
*/
package org.apache.drill.exec.store;
+import com.fasterxml.jackson.annotation.JsonProperty;
import org.apache.drill.common.logical.StoragePluginConfig;
import com.fasterxml.jackson.annotation.JsonTypeName;
-@JsonTypeName("named")
+@JsonTypeName(NamedStoragePluginConfig.NAME)
public class NamedStoragePluginConfig extends StoragePluginConfig {
- public String name;
+
+ public static final String NAME = "named";
+
+ private final String name;
+
+ public NamedStoragePluginConfig(@JsonProperty("name") String name) {
+this.name = name;
+ }
+
+ public String getName() {
+return name;
+ }
@Override
- public boolean equals(Object o) {
-return this == o;
+ public boolean equals(Object obj) {
+if (this == obj) {
+ return true;
+}
+if (obj == null) {
+ return false;
+}
+if (getClass() != obj.getClass()) {
+ return false;
+}
+NamedStoragePluginConfig other = (NamedStoragePluginConfig) obj;
+if (name == null) {
+ return other.name == null;
+} else {
+ return name.equals(other.name);
+}
}
@Override
public int hashCode() {
-return name.hashCode();
+final int prime = 31;
+int result = 1;
+result = prime * result + ((name == null) ? 0 : name.hashCode());
Review comment:
how about just `return (prime + ((name == null) ? 0 : name.hashCode());`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins Handler
sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins
Handler
URL: https://github.com/apache/drill/pull/1345#discussion_r199305514
##
File path:
contrib/storage-kafka/src/main/resources/bootstrap-storage-plugins.json
##
@@ -2,8 +2,8 @@
"storage":{
kafka : {
type:"kafka",
- enabled: false,
- kafkaConsumerProps: {"bootstrap.servers":"localhost:9092", "group.id" :
"drill-consumer"}
+ kafkaConsumerProps: {"bootstrap.servers":"localhost:9092", "group.id" :
"drill-consumer"},
+ enabled: false
Review comment:
is there any specific reason why this `enabled` setting is moved to end for
all the plugins ?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins Handler
sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins
Handler
URL: https://github.com/apache/drill/pull/1345#discussion_r199307586
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginsHandlerService.java
##
@@ -0,0 +1,130 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.base.Charsets;
+import com.google.common.io.Resources;
+import com.jasonclawson.jackson.dataformat.hocon.HoconFactory;
+import org.apache.drill.common.config.CommonConstants;
+import org.apache.drill.common.config.LogicalPlanPersistence;
+import org.apache.drill.common.exceptions.DrillRuntimeException;
+import org.apache.drill.common.logical.StoragePluginConfig;
+import org.apache.drill.common.scanner.ClassPathScanner;
+import org.apache.drill.exec.planner.logical.StoragePlugins;
+import org.apache.drill.exec.server.DrillbitContext;
+import org.apache.drill.exec.store.sys.PersistentStore;
+
+import javax.annotation.Nullable;
+import javax.validation.constraints.NotNull;
+import java.io.IOException;
+import java.net.URL;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+
+/**
+ * Drill plugins handler, which allows to update storage plugins configs from
the
+ * {@link CommonConstants#STORAGE_PLUGINS_OVERRIDE_CONF} conf file
+ *
+ * TODO: DRILL-6564: It can be improved with configs versioning and service of
creating
+ * {@link CommonConstants#STORAGE_PLUGINS_OVERRIDE_CONF}
+ */
+public class StoragePluginsHandlerService implements StoragePluginsHandler {
+ private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(StoragePluginsHandlerService.class);
+
+ private final LogicalPlanPersistence hoconLogicalPlanPersistence;
+
+ public StoragePluginsHandlerService(DrillbitContext context) {
+hoconLogicalPlanPersistence = new
LogicalPlanPersistence(context.getConfig(), context.getClasspathScan(),
+new ObjectMapper(new HoconFactory()));
+ }
+
+ @Override
+ public void loadPlugins(@NotNull PersistentStore
persistentStore,
+ @Nullable StoragePlugins bootstrapPlugins) {
+// if bootstrapPlugins is not null -- fresh Drill set up
+StoragePlugins pluginsToBeWrittenToPersistentStore;
+
+StoragePlugins newPlugins = getNewStoragePlugins();
+
+if (newPlugins != null) {
+ pluginsToBeWrittenToPersistentStore = new StoragePlugins(new
HashMap<>());
+ Optional.ofNullable(bootstrapPlugins)
+ .ifPresent(pluginsToBeWrittenToPersistentStore::putAll);
+
+ for (Map.Entry newPlugin : newPlugins) {
+String pluginName = newPlugin.getKey();
+StoragePluginConfig oldPluginConfig =
Optional.ofNullable(bootstrapPlugins)
+.map(plugins -> plugins.getConfig(pluginName))
+.orElse(persistentStore.get(pluginName));
+StoragePluginConfig updatedStatusPluginConfig =
updatePluginStatus(oldPluginConfig, newPlugin.getValue());
+pluginsToBeWrittenToPersistentStore.put(pluginName,
updatedStatusPluginConfig);
+ }
+} else {
+ pluginsToBeWrittenToPersistentStore = bootstrapPlugins;
+}
+
+// load pluginsToBeWrittenToPersistentStore to Persistent Store
+Optional.ofNullable(pluginsToBeWrittenToPersistentStore)
+.ifPresent(plugins -> plugins.forEach(plugin ->
persistentStore.put(plugin.getKey(), plugin.getValue(;
+ }
+
+ /**
+ * Helper method to identify the enabled status for new storage plugins
config. If this status is absent in the updater
+ * file, the status is kept from the configs, which are going to be updated
+ *
+ * @param oldPluginConfig current storage plugin config from Persistent
Store or bootstrap config file
+ * @param newPluginConfig new storage plugin config
+ * @return new storage plugin config with updated enabled status
+ */
+ private StoragePluginConfig updatePluginStatus(@Nullable StoragePluginConfig
oldPluginConfig,
+ @NotNull StoragePluginConfig
newPluginConfig) {
+if
[GitHub] sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins Handler
sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins
Handler
URL: https://github.com/apache/drill/pull/1345#discussion_r199306162
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginsHandler.java
##
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store;
+
+import org.apache.drill.common.logical.StoragePluginConfig;
+import org.apache.drill.exec.planner.logical.StoragePlugins;
+import org.apache.drill.exec.store.sys.PersistentStore;
+
+
+/**
+ * Storage plugins handler is an additional service for updating storage
plugins configs from the file
+ */
+public interface StoragePluginsHandler {
+
+ /**
+ * Update incoming storage plugins configs from persistence store if
present, otherwise bootstrap plugins configs.
+ * One of the params should be null, second shouldn't
+ *
+ * @param persistentStore the last storage plugins configs from persistence
store
+ * @param bootstrapPlugins bootstrap storage plugins, which are used in case
of first Drill start up
+ * @return all storage plugins, which should be loaded into persistence store
+ */
+ void loadPlugins(PersistentStore persistentStore,
StoragePlugins bootstrapPlugins);
Review comment:
It would be cleaner if `loadPlugins` accepts all the `storagePluginConfig`
instead of store and bootstrapPlugins. The handler should behave as
`loadPlugins` is getting existing plugin and then returning updated plugins.
The caller will decide how to pass existing plugin i.e. either from store or
bootstrap file and what to do with updated plugins.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins Handler
sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins
Handler
URL: https://github.com/apache/drill/pull/1345#discussion_r198912485
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistryImpl.java
##
@@ -63,17 +65,15 @@
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.cache.RemovalListener;
-import com.google.common.cache.RemovalNotification;
-import com.google.common.collect.Lists;
-import com.google.common.collect.Maps;
-import com.google.common.collect.Sets;
import com.google.common.io.Resources;
public class StoragePluginRegistryImpl implements StoragePluginRegistry {
private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(StoragePluginRegistryImpl.class);
+ /** Drill supported plugins */
private Map> availablePlugins =
Collections.emptyMap();
- private final StoragePluginMap plugins = new StoragePluginMap();
+ /** Enabled plugins */
Review comment:
For single line comments please use `//` here and above
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins Handler
sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins Handler URL: https://github.com/apache/drill/pull/1345#discussion_r198902957 ## File path: distribution/src/deb/control/conffiles ## @@ -16,3 +16,4 @@ /etc/drill/conf/drill-override.conf /etc/drill/conf/logback.xml /etc/drill/conf/drill-env.sh +/etc/drill/conf/storage-plugins-updates.conf Review comment: This file name is different than the constant defined "STORAGE_PLUGINS_UPDATER_FILE". And what is the purpose of putting this file here ? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins Handler
sohami commented on a change in pull request #1345: DRILL-6494: Drill Plugins Handler URL: https://github.com/apache/drill/pull/1345#discussion_r198912059 ## File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistry.java ## @@ -83,15 +84,13 @@ * @param name * @param plugin */ - void addPlugin(String name, StoragePlugin plugin); + void addPluginToEnabled(String name, StoragePlugin plugin); Review comment: should it be `addEnabledPlugins` instead ? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
