You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2021/07/22 21:04:03 UTC
[GitHub] [drill] vdiravka opened a new pull request #2278: DRILL-7975: Connection to Splunk Drill Storage Plugin fails intermittently
vdiravka opened a new pull request #2278:
URL: https://github.com/apache/drill/pull/2278
# [DRILL-7975](https://issues.apache.org/jira/browse/DRILL-7975): Connection to Splunk Drill Storage Plugin fails intermittently
## Description
Introduces the new filed in Plugin config, which allows to specify the number of retries of connections to data source. Actually works only for Splunk and can be easily extended for other data sources.
## Documentation
`reconnectRetries` - the plugin config property. Determines the number of retries of connections to data source in case the previous ones were unsuccessful.
## Testing
Covered with Unit test
--
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
[GitHub] [drill] vvysotskyi commented on a change in pull request #2278: DRILL-7975: Connection to Splunk Drill Storage Plugin fails intermittently
Posted by 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
[GitHub] [drill] vdiravka commented on a change in pull request #2278: DRILL-7975: Connection to Splunk Drill Storage Plugin fails intermittently
Posted by 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
[GitHub] [drill] luocooong commented on a change in pull request #2278: DRILL-7975: Connection to Splunk Drill Storage Plugin fails intermittently
Posted by GitBox <gi...@apache.org>.
luocooong commented on a change in pull request #2278:
URL: https://github.com/apache/drill/pull/2278#discussion_r675481061
##########
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:
Does it need to sleeping for a few seconds?
##########
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:
Is it possible to put the `enabled` at the end?
--
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
[GitHub] [drill] vdiravka merged pull request #2278: DRILL-7975: Connection to Splunk Drill Storage Plugin fails intermittently
Posted by GitBox <gi...@apache.org>.
vdiravka merged pull request #2278:
URL: https://github.com/apache/drill/pull/2278
--
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
[GitHub] [drill] vvysotskyi commented on a change in pull request #2278: DRILL-7975: Connection to Splunk Drill Storage Plugin fails intermittently
Posted by GitBox <gi...@apache.org>.
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on pull request #2278: DRILL-7975: Connection to Splunk Drill Storage Plugin fails intermittently
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on pull request #2278:
URL: https://github.com/apache/drill/pull/2278#issuecomment-886925937
Ok, then let's revert those changes here and address the issue in DRILL-7973, since currently at least the master branch is green.
--
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
[GitHub] [drill] vdiravka commented on pull request #2278: DRILL-7975: Connection to Splunk Drill Storage Plugin fails intermittently
Posted by GitBox <gi...@apache.org>.
vdiravka commented on pull request #2278:
URL: https://github.com/apache/drill/pull/2278#issuecomment-886918399
> @vdiravka, thanks for making changes, +1
>
> **Let's merge it only after CI jobs passed to ensure that memory changes were fine.**
Memory changes improved the CI builds a bit, but not fully. I will try to cover it in [DRILL-7973](https://github.com/vdiravka/drill/tree/DRILL-7973)
--
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
[GitHub] [drill] vvysotskyi commented on a change in pull request #2278: DRILL-7975: Connection to Splunk Drill Storage Plugin fails intermittently
Posted by GitBox <gi...@apache.org>.
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:
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... If someone else will be willing to use it in one more plugin, it is possible to move it to the common parent class, but for now, it is better to leave it in Splunk config only.
--
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