[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make format plugins fully pluggable
arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make
format plugins fully pluggable
URL: https://github.com/apache/drill/pull/1780#discussion_r281531418
##
File path:
exec/java-exec/src/test/java/org/apache/drill/exec/store/TestFormatPluginLoading.java
##
@@ -0,0 +1,111 @@
+/*
+ * 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.ExecConstants;
+import org.apache.drill.exec.ExecTest;
+import org.apache.drill.exec.store.dfs.FileSystemConfig;
+import org.apache.drill.exec.store.easy.text.TextFormatPlugin;
+import org.apache.drill.exec.store.sys.PersistentStore;
+import org.apache.drill.shaded.guava.com.google.common.io.Resources;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.File;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class TestFormatPluginLoading extends ExecTest {
+ private final StoragePluginRegistryImpl storagePluginRegistry;
+ private final PersistentStore plugins;
+
+ private static File storagePlugins;
+ private static File formatPlugins;
+
+ public TestFormatPluginLoading() throws Exception {
+storagePluginRegistry = new
StoragePluginRegistryImpl(mockDrillbitContext());
+storagePluginRegistry.init();
+plugins = storagePluginRegistry.getStore();
+ }
+
+ @BeforeClass
+ public static void setup() {
+storagePlugins = new File(Resources.getResource("store/" +
ExecConstants.BOOTSTRAP_STORAGE_PLUGINS_FILE).getFile());
Review comment:
Can you please explain what you are doing here? I am sure what this renaming
logic does.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make format plugins fully pluggable
arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make
format plugins fully pluggable
URL: https://github.com/apache/drill/pull/1780#discussion_r280988558
##
File path: exec/java-exec/src/test/resources/bootstrap-format-plugins.json
##
@@ -0,0 +1,31 @@
+{
Review comment:
Please try to use these files only for your unit tests so other tests do not
get affected.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make format plugins fully pluggable
arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make
format plugins fully pluggable
URL: https://github.com/apache/drill/pull/1780#discussion_r280835496
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistryImpl.java
##
@@ -334,6 +331,48 @@ private StoragePlugins
loadBootstrapPlugins(LogicalPlanPersistence lpPersistence
}
}
+ private void loadStoragePlugins(URL url, StoragePlugins bootstrapPlugins,
Map pluginURLMap, LogicalPlanPersistence lpPersistence) throws
IOException {
+StoragePlugins plugins = getPluginsFromResource(url, lpPersistence);
+plugins.forEach(plugin -> {
+ StoragePluginConfig oldPluginConfig =
bootstrapPlugins.putIfAbsent(plugin.getKey(), plugin.getValue());
+ if (oldPluginConfig != null) {
+logger.warn("Duplicate plugin instance '{}' defined in [{}, {}],
ignoring the later one.",
+plugin.getKey(), pluginURLMap.get(plugin.getKey()), url);
+ } else {
+pluginURLMap.put(plugin.getKey(), url);
+ }
+});
+ }
+
+ private void loadFormatPlugins(URL url, StoragePlugins bootstrapPlugins,
Map pluginURLMap, LogicalPlanPersistence lpPersistence) throws
IOException {
Review comment:
Please add Javadoc.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make format plugins fully pluggable
arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make
format plugins fully pluggable
URL: https://github.com/apache/drill/pull/1780#discussion_r280832129
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemConfig.java
##
@@ -48,7 +49,7 @@ public FileSystemConfig(@JsonProperty("connection") String
connection,
Map caseInsensitiveWorkspaces =
CaseInsensitiveMap.newHashMap();
Optional.ofNullable(workspaces).ifPresent(caseInsensitiveWorkspaces::putAll);
this.workspaces = caseInsensitiveWorkspaces;
-this.formats = formats;
+this.formats = formats != null? formats : new LinkedHashMap<>();
Review comment:
```suggestion
this.formats = formats != null ? formats : new LinkedHashMap<>();
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make format plugins fully pluggable
arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make
format plugins fully pluggable
URL: https://github.com/apache/drill/pull/1780#discussion_r280834454
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistryImpl.java
##
@@ -334,6 +331,48 @@ private StoragePlugins
loadBootstrapPlugins(LogicalPlanPersistence lpPersistence
}
}
+ private void loadStoragePlugins(URL url, StoragePlugins bootstrapPlugins,
Map pluginURLMap, LogicalPlanPersistence lpPersistence) throws
IOException {
+StoragePlugins plugins = getPluginsFromResource(url, lpPersistence);
+plugins.forEach(plugin -> {
+ StoragePluginConfig oldPluginConfig =
bootstrapPlugins.putIfAbsent(plugin.getKey(), plugin.getValue());
+ if (oldPluginConfig != null) {
+logger.warn("Duplicate plugin instance '{}' defined in [{}, {}],
ignoring the later one.",
+plugin.getKey(), pluginURLMap.get(plugin.getKey()), url);
+ } else {
+pluginURLMap.put(plugin.getKey(), url);
+ }
+});
+ }
+
+ private void loadFormatPlugins(URL url, StoragePlugins bootstrapPlugins,
Map pluginURLMap, LogicalPlanPersistence lpPersistence) throws
IOException {
+StoragePlugins plugins = getPluginsFromResource(url, lpPersistence);
+plugins.forEach(formatPlugin -> {
+ String targetStoragePluginName = formatPlugin.getKey();
+ StoragePluginConfig storagePlugin =
bootstrapPlugins.getConfig(targetStoragePluginName);
+ StoragePluginConfig formatPluginValue = formatPlugin.getValue();
+ if (storagePlugin == null) {
+logger.warn("No storage plugins with the given name are registered:
'{}'", targetStoragePluginName);
+ } else if (storagePlugin instanceof FileSystemConfig &&
formatPluginValue instanceof FileSystemConfig) {
+FileSystemConfig targetPlugin = (FileSystemConfig) storagePlugin;
+((FileSystemConfig)
formatPluginValue).getFormats().forEach((formatName, formatValue) -> {
+ if (targetPlugin.getFormats().containsKey(formatName)) {
Review comment:
Use putIfAbsent as in `loadStoragePlugins`
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make format plugins fully pluggable
arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make
format plugins fully pluggable
URL: https://github.com/apache/drill/pull/1780#discussion_r280834206
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistryImpl.java
##
@@ -334,6 +331,48 @@ private StoragePlugins
loadBootstrapPlugins(LogicalPlanPersistence lpPersistence
}
}
+ private void loadStoragePlugins(URL url, StoragePlugins bootstrapPlugins,
Map pluginURLMap, LogicalPlanPersistence lpPersistence) throws
IOException {
Review comment:
Please add Javadoc.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make format plugins fully pluggable
arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make
format plugins fully pluggable
URL: https://github.com/apache/drill/pull/1780#discussion_r280834695
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistryImpl.java
##
@@ -334,6 +331,48 @@ private StoragePlugins
loadBootstrapPlugins(LogicalPlanPersistence lpPersistence
}
}
+ private void loadStoragePlugins(URL url, StoragePlugins bootstrapPlugins,
Map pluginURLMap, LogicalPlanPersistence lpPersistence) throws
IOException {
+StoragePlugins plugins = getPluginsFromResource(url, lpPersistence);
+plugins.forEach(plugin -> {
+ StoragePluginConfig oldPluginConfig =
bootstrapPlugins.putIfAbsent(plugin.getKey(), plugin.getValue());
+ if (oldPluginConfig != null) {
+logger.warn("Duplicate plugin instance '{}' defined in [{}, {}],
ignoring the later one.",
+plugin.getKey(), pluginURLMap.get(plugin.getKey()), url);
+ } else {
+pluginURLMap.put(plugin.getKey(), url);
+ }
+});
+ }
+
+ private void loadFormatPlugins(URL url, StoragePlugins bootstrapPlugins,
Map pluginURLMap, LogicalPlanPersistence lpPersistence) throws
IOException {
+StoragePlugins plugins = getPluginsFromResource(url, lpPersistence);
+plugins.forEach(formatPlugin -> {
+ String targetStoragePluginName = formatPlugin.getKey();
+ StoragePluginConfig storagePlugin =
bootstrapPlugins.getConfig(targetStoragePluginName);
+ StoragePluginConfig formatPluginValue = formatPlugin.getValue();
+ if (storagePlugin == null) {
+logger.warn("No storage plugins with the given name are registered:
'{}'", targetStoragePluginName);
Review comment:
```suggestion
logger.warn("No storage plugins with the given name are registered:
[{}]", targetStoragePluginName);
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make format plugins fully pluggable
arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make
format plugins fully pluggable
URL: https://github.com/apache/drill/pull/1780#discussion_r280834770
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistryImpl.java
##
@@ -334,6 +331,48 @@ private StoragePlugins
loadBootstrapPlugins(LogicalPlanPersistence lpPersistence
}
}
+ private void loadStoragePlugins(URL url, StoragePlugins bootstrapPlugins,
Map pluginURLMap, LogicalPlanPersistence lpPersistence) throws
IOException {
+StoragePlugins plugins = getPluginsFromResource(url, lpPersistence);
+plugins.forEach(plugin -> {
+ StoragePluginConfig oldPluginConfig =
bootstrapPlugins.putIfAbsent(plugin.getKey(), plugin.getValue());
+ if (oldPluginConfig != null) {
+logger.warn("Duplicate plugin instance '{}' defined in [{}, {}],
ignoring the later one.",
Review comment:
```suggestion
logger.warn("Duplicate plugin instance [{}] defined in [{}, {}],
ignoring the later one.",
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make format plugins fully pluggable
arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make
format plugins fully pluggable
URL: https://github.com/apache/drill/pull/1780#discussion_r280834627
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistryImpl.java
##
@@ -334,6 +331,48 @@ private StoragePlugins
loadBootstrapPlugins(LogicalPlanPersistence lpPersistence
}
}
+ private void loadStoragePlugins(URL url, StoragePlugins bootstrapPlugins,
Map pluginURLMap, LogicalPlanPersistence lpPersistence) throws
IOException {
+StoragePlugins plugins = getPluginsFromResource(url, lpPersistence);
+plugins.forEach(plugin -> {
+ StoragePluginConfig oldPluginConfig =
bootstrapPlugins.putIfAbsent(plugin.getKey(), plugin.getValue());
+ if (oldPluginConfig != null) {
+logger.warn("Duplicate plugin instance '{}' defined in [{}, {}],
ignoring the later one.",
+plugin.getKey(), pluginURLMap.get(plugin.getKey()), url);
+ } else {
+pluginURLMap.put(plugin.getKey(), url);
+ }
+});
+ }
+
+ private void loadFormatPlugins(URL url, StoragePlugins bootstrapPlugins,
Map pluginURLMap, LogicalPlanPersistence lpPersistence) throws
IOException {
+StoragePlugins plugins = getPluginsFromResource(url, lpPersistence);
+plugins.forEach(formatPlugin -> {
+ String targetStoragePluginName = formatPlugin.getKey();
+ StoragePluginConfig storagePlugin =
bootstrapPlugins.getConfig(targetStoragePluginName);
+ StoragePluginConfig formatPluginValue = formatPlugin.getValue();
+ if (storagePlugin == null) {
+logger.warn("No storage plugins with the given name are registered:
'{}'", targetStoragePluginName);
+ } else if (storagePlugin instanceof FileSystemConfig &&
formatPluginValue instanceof FileSystemConfig) {
+FileSystemConfig targetPlugin = (FileSystemConfig) storagePlugin;
+((FileSystemConfig)
formatPluginValue).getFormats().forEach((formatName, formatValue) -> {
+ if (targetPlugin.getFormats().containsKey(formatName)) {
+logger.warn("Duplicate format instance '{}' defined in [{}, {}],
ignoring the later one.",
+formatName, pluginURLMap.get(targetStoragePluginName), url);
+ } else {
+targetPlugin.getFormats().put(formatName, formatValue);
+ }
+});
+ } else {
+logger.warn("Formats are only supported by File System plugin type:
'{}'", targetStoragePluginName);
Review comment:
```suggestion
logger.warn("Formats are only supported by File System plugin type:
[{}]", targetStoragePluginName);
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make format plugins fully pluggable
arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make
format plugins fully pluggable
URL: https://github.com/apache/drill/pull/1780#discussion_r280833295
##
File path: exec/java-exec/src/test/resources/bootstrap-format-plugins.json
##
@@ -0,0 +1,31 @@
+{
Review comment:
Will the below formats be loaded for all tests since they are present in the
classpath or only for the specific unit tests?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make format plugins fully pluggable
arina-ielchiieva commented on a change in pull request #1780: DRILL-7030: Make
format plugins fully pluggable
URL: https://github.com/apache/drill/pull/1780#discussion_r280834121
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistryImpl.java
##
@@ -310,22 +311,18 @@ public void addPluginToPersistentStoreIfAbsent(String
name, StoragePluginConfig
private StoragePlugins loadBootstrapPlugins(LogicalPlanPersistence
lpPersistence) 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);
+Set storageUrls =
ClassPathScanner.forResource(ExecConstants.BOOTSTRAP_STORAGE_PLUGINS_FILE,
false);
+Set formatUrls =
ClassPathScanner.forResource(ExecConstants.BOOTSTRAP_FORMAT_PLUGINS_FILE,
false);
+if (storageUrls != null && !storageUrls.isEmpty()) {
+ logger.info("Loading the storage plugin configs from URLs {}.",
storageUrls);
StoragePlugins bootstrapPlugins = new StoragePlugins(new HashMap<>());
Map pluginURLMap = new HashMap<>();
- for (URL url : urls) {
-String pluginsData = Resources.toString(url, Charsets.UTF_8);
-StoragePlugins plugins =
lpPersistence.getMapper().readValue(pluginsData, StoragePlugins.class);
-for (Entry plugin : plugins) {
- StoragePluginConfig oldPluginConfig =
bootstrapPlugins.putIfAbsent(plugin.getKey(), plugin.getValue());
- if (oldPluginConfig != null) {
-logger.warn("Duplicate plugin instance '{}' defined in [{}, {}],
ignoring the later one.",
-plugin.getKey(), pluginURLMap.get(plugin.getKey()), url);
- } else {
-pluginURLMap.put(plugin.getKey(), url);
- }
+ for (URL url : storageUrls) {
Review comment:
add log info, similar as for storage plugins
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:
us...@infra.apache.org
With regards,
Apache Git Services
