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/10/20 09:31:43 UTC
[GitHub] [drill] dzamo opened a new pull request #2342: DRILL-8016: Default to lazy outbound connections for storage-jdbc and storage-splunk
dzamo opened a new pull request #2342:
URL: https://github.com/apache/drill/pull/2342
# [DRILL-8016](https://issues.apache.org/jira/browse/DRILL-8016): Default to lazy outbound connections for storage-jdbc and storage-splunk
## Description
The only two of our storage plugins that try to connect out eagerly are storage-splunk and storage-jdbc. This makes both of them lazy and sets default HikariCP parameters that make Drill more thrifty about bringing up outbound connections.
## Documentation
Mention the considerations behind the HikariCP default parameters and that users can override them.
## Testing
Unit tests already present, interactive testing with different HikariCP parameters against a pg database.
--
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 pull request #2342: DRILL-8016: Default to lazy outbound connections for storage-jdbc and storage-splunk
Posted by GitBox <gi...@apache.org>.
luocooong commented on pull request #2342:
URL: https://github.com/apache/drill/pull/2342#issuecomment-947608751
@dzamo Thanks for the contribution. I could take a look at it tomorrow.
--
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 #2342: DRILL-8016: Default to lazy outbound connections for storage-jdbc and storage-splunk
Posted by GitBox <gi...@apache.org>.
luocooong commented on a change in pull request #2342:
URL: https://github.com/apache/drill/pull/2342#discussion_r733734268
##########
File path: contrib/storage-jdbc/src/main/resources/bootstrap-storage-plugins.json
##########
@@ -8,6 +8,10 @@
"password": "xxx",
"caseInsensitiveTableNames": false,
"sourceParameters" : {
+ "idleTimeout": 3600000,
Review comment:
The "old" does not included this patch. So, you can build code on the master (mark to old), then build the new version on your patch branch.
Actually, we add the parameter is in the Map data structure and there should be no problem, but it's better to test it.
--
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] dzamo commented on a change in pull request #2342: DRILL-8016: Default to lazy outbound connections for storage-jdbc and storage-splunk
Posted by GitBox <gi...@apache.org>.
dzamo commented on a change in pull request #2342:
URL: https://github.com/apache/drill/pull/2342#discussion_r733928791
##########
File path: contrib/storage-jdbc/src/main/resources/bootstrap-storage-plugins.json
##########
@@ -8,6 +8,10 @@
"password": "xxx",
"caseInsensitiveTableNames": false,
"sourceParameters" : {
+ "idleTimeout": 3600000,
Review comment:
@luocooong Okay I followed your steps using a public pg database (details below, not sensitive). The Drill version used to create the storage config was 1.18 (just because I have it lying around) then I started Drill built from this branch and the storage config was present and enabled. I disabled and enabled it again without errors.
```
{
"type": "jdbc",
"driver": "org.postgresql.Driver",
"url": "jdbc:postgresql://hh-pgsql-public.ebi.ac.uk:5432/pfmegrnargs",
"username": "reader",
"password": "NWDMCE5xdipIjRrp",
"caseInsensitiveTableNames": false,
"sourceParameters": {
"maximumPoolSize": 1
},
"enabled": true
}
```
--
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] dzamo commented on a change in pull request #2342: DRILL-8016: Default to lazy outbound connections for storage-jdbc and storage-splunk
Posted by GitBox <gi...@apache.org>.
dzamo commented on a change in pull request #2342:
URL: https://github.com/apache/drill/pull/2342#discussion_r733692989
##########
File path: contrib/storage-jdbc/src/main/resources/bootstrap-storage-plugins.json
##########
@@ -8,6 +8,10 @@
"password": "xxx",
"caseInsensitiveTableNames": false,
"sourceParameters" : {
+ "idleTimeout": 3600000,
Review comment:
@luocooong Oh interesting. Is it okay if I test with Drill 1.18 for the old version? And use storage configs in files stored by drill-embedded, instead of ZooKeeper? The drill-embedded of the new Drill version will go and look for the config files in the same location...
--
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 #2342: DRILL-8016: Default to lazy outbound connections for storage-jdbc and storage-splunk
Posted by GitBox <gi...@apache.org>.
luocooong commented on a change in pull request #2342:
URL: https://github.com/apache/drill/pull/2342#discussion_r733684076
##########
File path: contrib/storage-jdbc/src/main/resources/bootstrap-storage-plugins.json
##########
@@ -8,6 +8,10 @@
"password": "xxx",
"caseInsensitiveTableNames": false,
"sourceParameters" : {
+ "idleTimeout": 3600000,
Review comment:
I'm not sure if there exist an incompatible after the boot file is upgraded. Please check that :
1. boot Drill (with standalone ZK) using old version
2. create a JDBC storage
3. shutdown and upgrade the Drill
5. reboot the Drill & do check
--
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] dzamo merged pull request #2342: DRILL-8016: Default to lazy outbound connections for storage-jdbc and storage-splunk
Posted by GitBox <gi...@apache.org>.
dzamo merged pull request #2342:
URL: https://github.com/apache/drill/pull/2342
--
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] cgivre commented on a change in pull request #2342: DRILL-8016: Default to lazy outbound connections for storage-jdbc and storage-splunk
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2342:
URL: https://github.com/apache/drill/pull/2342#discussion_r732769408
##########
File path: contrib/storage-splunk/src/main/java/org/apache/drill/exec/store/splunk/SplunkSchemaFactory.java
##########
@@ -40,19 +38,11 @@
private static final Logger logger = LoggerFactory.getLogger(SplunkSchemaFactory.class);
private static final String SPL_TABLE_NAME = "spl";
private final SplunkStoragePlugin plugin;
- private final EntityCollection<Index> indexes;
+ // private final EntityCollection<Index> indexes;
Review comment:
Please remove commented out code.
--
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] dzamo commented on a change in pull request #2342: DRILL-8016: Default to lazy outbound connections for storage-jdbc and storage-splunk
Posted by GitBox <gi...@apache.org>.
dzamo commented on a change in pull request #2342:
URL: https://github.com/apache/drill/pull/2342#discussion_r733928791
##########
File path: contrib/storage-jdbc/src/main/resources/bootstrap-storage-plugins.json
##########
@@ -8,6 +8,10 @@
"password": "xxx",
"caseInsensitiveTableNames": false,
"sourceParameters" : {
+ "idleTimeout": 3600000,
Review comment:
Okay I followed your steps using a public pg database (details below, not sensitive). The Drill version used to create the storage config was 1.18 (just because I have it lying around) then I started Drill built from this branch and the storage config was present and enabled. I disabled and enabled it again without errors.
```
{
"type": "jdbc",
"driver": "org.postgresql.Driver",
"url": "jdbc:postgresql://hh-pgsql-public.ebi.ac.uk:5432/pfmegrnargs",
"username": "reader",
"password": "NWDMCE5xdipIjRrp",
"caseInsensitiveTableNames": false,
"sourceParameters": {
"maximumPoolSize": 1
},
"enabled": true
}
```
--
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] dzamo commented on pull request #2342: DRILL-8016: Default to lazy outbound connections for storage-jdbc and storage-splunk
Posted by GitBox <gi...@apache.org>.
dzamo commented on pull request #2342:
URL: https://github.com/apache/drill/pull/2342#issuecomment-948448967
@cgivre, thank you. I've updated the docs, removed the dead code and uncommented the line which depends on JDBC writer. That last step means I can't push something that builds until we've merged JDBC writer and I've rebased, so I'll keep this PR on hold until then.
--
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