[GitHub] [drill] vdiravka commented on a change in pull request #2278: DRILL-7975: Connection to Splunk Drill Storage Plugin fails intermittently

[GitBox <gi...@apache.org>]

vdiravka commented on a change in pull request #2278:
URL: https://github.com/apache/drill/pull/2278#discussion_r675404571



##########
File path: contrib/storage-splunk/src/test/java/org/apache/drill/exec/store/splunk/SplunkTestSuite.java
##########
@@ -71,7 +79,9 @@ public static void initSplunk() throws Exception {
         String hostname = splunk.getHost();
         Integer port = splunk.getFirstMappedPort();
         StoragePluginRegistry pluginRegistry = cluster.drillbit().getContext().getStorage();
-        SPLUNK_STORAGE_PLUGIN_CONFIG = new SplunkPluginConfig(SPLUNK_LOGIN, SPLUNK_PASS, hostname, port, "1", "now", null);
+        JsonNode storagePluginJson = mapper.readTree(new File(Resources.getResource("bootstrap-storage-plugins.json").toURI()));
+        SPLUNK_STORAGE_PLUGIN_CONFIG = mapper.treeToValue(storagePluginJson.get("storage").get("splunk"), SplunkPluginConfig.class);
+        setPort(SPLUNK_STORAGE_PLUGIN_CONFIG, port);

Review comment:
       1. The main reason is:
   In the runtime we don't use the constructor for instantiating PluginConfigs. We deserialize them from JSON.
   So right now the tests and `SplunkTestSuite.initSplunk()` reflects this too.
   2. The other reason not all configs can be specified via constructor.

##########
File path: logical/src/main/java/org/apache/drill/common/logical/StoragePluginConfig.java
##########
@@ -29,6 +31,13 @@
   // DO NOT include enabled status in equality and hash
   // comparisons; doing so will break the plugin registry.
   private Boolean enabled;
+  protected final Integer reconnectRetries;

Review comment:
       It is better to place it here, because this can be used for any other datasource event now. It is easy to add that for any of them this functionality. But since the implementation for any of them is specific and requires test cases, I didn't add changes for other plugins connections.
   Therefore it is a common config for all plugins, like `enabled`

##########
File path: contrib/storage-splunk/src/main/java/org/apache/drill/exec/store/splunk/SplunkConnection.java
##########
@@ -71,10 +73,13 @@ public Service connect() {
     loginArgs.setPort(port);
     loginArgs.setPassword(credentials.getPassword());
     loginArgs.setUsername(credentials.getUsername());
-
     try {
+      connectionAttempts--;
       service = Service.connect(loginArgs);
     } catch (Exception e) {

Review comment:
       Good suggestion. I'll add:
   `TimeUnit.SECONDS.sleep(2);`

##########
File path: contrib/storage-splunk/src/main/java/org/apache/drill/exec/store/splunk/SplunkConnection.java
##########
@@ -41,11 +41,14 @@
   private final String hostname;
   private final int port;
   private Service service;
+  private Integer reconnectRetries;
+  private int counter;

Review comment:
       Nice suggestion. Will do.
   Thanks!

##########
File path: contrib/storage-splunk/src/main/resources/bootstrap-storage-plugins.json
##########
@@ -8,7 +8,8 @@
       "port": 8089,
       "earliestTime": "-14d",
       "latestTime": "now",
-      "enabled": false
+      "enabled": false,

Review comment:
       What about placing in the beginning of the config?




-- 
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.

To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org