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

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

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



##########
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:
       This property is used for the Splunk plugin only, maybe it makes sense to move it there instead of declaring in a base class?

##########
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:
       We could use a single variable here, for example, `connectionAttempts`, decrease its value every time we are trying to connect and check in the catch block whether it is larger than 0.

##########
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:
       Could you please explain the reason for this change? The initial version where the constructor is used looks more clear and simple.




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