vvysotskyi commented on a change in pull request #2278:
URL: https://github.com/apache/drill/pull/2278#discussion_r675621464
##########
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:
But some plugins might not use this property, or it may be invalid for
them, for example, there is no reason to use it for `sys` or `cp` plugins, but
they all will have it...
##########
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. But the way you are deserializing slightly differs from how it is
deserialized in runtime. For example, we don't hack port at runtime, use
configured object mapper, etc. Also, you are setting the port number through
the reflection, but what if it wasn't deserialized correctly, so it wouldn't be
checked.
If we want to test the config deserializer for it, let's do it in a separate
test, but this change only makes things more complex.
2. I don't see how Jackson will set the correct `reconnectRetries` value to
the `SplunkPluginConfig` instance. It will call the `SplunkPluginConfig`
constructor, marked with `@JsonCreator` annotation, this constructor will call
parent one that has the hardcoded number of the `reconnectRetries` value, so no
actual value should be passed...
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
