You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by milindt <gi...@git.apache.org> on 2018/01/17 09:12:31 UTC
[GitHub] drill pull request #1094: DRILL-6090: While connecting to drill-bits using J...
GitHub user milindt opened a pull request:
https://github.com/apache/drill/pull/1094
DRILL-6090: While connecting to drill-bits using JDBC Driver through …
…Zookeeper, a lot of "Curator-Framework-0" threads are created if connection to drill-bit is not successful(no drill-bits are up/reachable)
I am using Drill JDBC driver 1.12.0 to connect to MapR-DB. I am finding the available drill-bits using Zookeepers. When drill-bits are not up or not reachable, the connection is failed with exception: "Failure in connecting to Drill: oadd.org.apache.drill.exec.rpc.RpcException: Failure setting up ZK for client", which is expected, but number of threads created by ZKClusterCoordinator just keeps on increasing.
Steps to reproduce the issue
Setup a connection with a drill-bit using Apache Drill JDBC driver 1.12.0 through Zookeeper hosts(port 5181)
Now stop the drill-bit services or block the drill-bit IPs using iptable rules
Truncate catalina logs
Try to connect to the drill-bit/hit a code path that requires connection to drill-bits.
Take thread dump using kill -QUIT <java process id>
grep -c "Curator-Framework-0" catalina.out
Observe that the curator framework thread just keep on accumulating
RCA:
ZKClusterCoordinator creates curator threads in the constructor
ZKClusterCoordinator is instantiated by DrillClient.connect
DrillClient.connect is called in DrillConnectionImpl constructor
Fix:
Call DrillConnectionImpl .cleanup() from all the catch blocks in the DrillConnectionImpl constructor.
[JIRA link](https://issues.apache.org/jira/browse/DRILL-6090)
If this fix is accepted, will there be a release from this branch(1.12.0)? A release with minor version may be.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/milindt/drill 1.12.0
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/1094.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1094
----
----
---
[GitHub] drill issue #1094: DRILL-6090: While connecting to drill-bits using JDBC Dri...
Posted by milindt <gi...@git.apache.org>.
Github user milindt commented on the issue:
https://github.com/apache/drill/pull/1094
@paul-rogers can you please review this fix?
---
[GitHub] drill pull request #1094: DRILL-6090: While connecting to drill-bits using J...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/1094#discussion_r162516548
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillConnectionImpl.java ---
@@ -132,10 +132,12 @@ protected DrillConnectionImpl(DriverImpl driver, AvaticaFactory factory,
bit = new Drillbit(dConfig, serviceSet);
bit.run();
} catch (final UserException e) {
+ cleanup();
throw new SQLException(
"Failure in starting embedded Drillbit: " + e.getMessage(),
e);
} catch (Exception e) {
+ cleanup();
--- End diff --
One trick is this: used nested exceptions. The inner set "parses" the exception types, the outer does cleanup:
```
try {
try {
// do something
} catch (ExceptionA e) {
// Do something
} catch (ExceptionB e) {
// Do something else
... }
} catch (Throwable t) {
cleanup();
throw t;
}
```
---
[GitHub] drill pull request #1094: DRILL-6090: While connecting to drill-bits using J...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/1094#discussion_r169214241
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillConnectionImpl.java ---
@@ -108,73 +108,81 @@ protected DrillConnectionImpl(DriverImpl driver, AvaticaFactory factory,
super.setReadOnly(false);
this.config = new DrillConnectionConfig(info);
+ try{
+ try {
+ String connect = null;
- try {
- String connect = null;
-
- if (config.isLocal()) {
- try {
- Class.forName("org.eclipse.jetty.server.Handler");
- } catch (final ClassNotFoundException e) {
- throw new SQLNonTransientConnectionException(
- "Running Drill in embedded mode using Drill's jdbc-all JDBC"
- + " driver Jar file alone is not supported.", e);
- }
-
- final DrillConfig dConfig = DrillConfig.create(info);
- this.allocator = RootAllocatorFactory.newRoot(dConfig);
- RemoteServiceSet set = GlobalServiceSetReference.SETS.get();
- if (set == null) {
- // We're embedded; start a local drill bit.
- serviceSet = RemoteServiceSet.getLocalServiceSet();
- set = serviceSet;
+ if (config.isLocal()) {
try {
- bit = new Drillbit(dConfig, serviceSet);
- bit.run();
- } catch (final UserException e) {
- throw new SQLException(
- "Failure in starting embedded Drillbit: " + e.getMessage(),
- e);
- } catch (Exception e) {
- // (Include cause exception's text in wrapping exception's text so
- // it's more likely to get to user (e.g., via SQLLine), and use
- // toString() since getMessage() text doesn't always mention error:)
- throw new SQLException("Failure in starting embedded Drillbit: " + e, e);
+ Class.forName("org.eclipse.jetty.server.Handler");
+ } catch (final ClassNotFoundException e) {
+ throw new SQLNonTransientConnectionException(
+ "Running Drill in embedded mode using Drill's jdbc-all JDBC"
+ + " driver Jar file alone is not supported.", e);
+ }
+
+ final DrillConfig dConfig = DrillConfig.create(info);
+ this.allocator = RootAllocatorFactory.newRoot(dConfig);
--- End diff --
Not your bug: error in original code. Drill JDBC allows creation of multiple connections. Each will have a separate root allocator. This might oversubscribe memory in extreme case. Probably need to file a JIRA for this case.
---
[GitHub] drill pull request #1094: DRILL-6090: While connecting to drill-bits using J...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1094#discussion_r162311140
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillConnectionImpl.java ---
@@ -132,10 +132,12 @@ protected DrillConnectionImpl(DriverImpl driver, AvaticaFactory factory,
bit = new Drillbit(dConfig, serviceSet);
bit.run();
} catch (final UserException e) {
+ cleanup();
throw new SQLException(
"Failure in starting embedded Drillbit: " + e.getMessage(),
e);
} catch (Exception e) {
+ cleanup();
--- End diff --
What if you use flag `doCleanup` which will be true by default and after `this.client.connect(connect, info);` succeedes, it would be set to false. In finally block you'll do clean up only if flag is true?
---
[GitHub] drill pull request #1094: DRILL-6090: While connecting to drill-bits using J...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/1094
---
[GitHub] drill issue #1094: DRILL-6090: While connecting to drill-bits using JDBC Dri...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1094
@milindt could you please address code review comments so changes can be pushed into master?
---
[GitHub] drill issue #1094: DRILL-6090: While connecting to drill-bits using JDBC Dri...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1094
@milindt release 1.12.0 is out. Regarding this fix, once you address code review comments it will be added in 1.13.0.
---
[GitHub] drill issue #1094: DRILL-6090: While connecting to drill-bits using JDBC Dri...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1094
+1, LGTM.
---
[GitHub] drill issue #1094: DRILL-6090: While connecting to drill-bits using JDBC Dri...
Posted by milindt <gi...@git.apache.org>.
Github user milindt commented on the issue:
https://github.com/apache/drill/pull/1094
Details in [DRILL-6090](https://issues.apache.org/jira/browse/DRILL-6090)
---
[GitHub] drill issue #1094: DRILL-6090: While connecting to drill-bits using JDBC Dri...
Posted by milindt <gi...@git.apache.org>.
Github user milindt commented on the issue:
https://github.com/apache/drill/pull/1094
@arina-ielchiieva, @paul-rogers can you please review?
---
[GitHub] drill pull request #1094: DRILL-6090: While connecting to drill-bits using J...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1094#discussion_r162282830
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillConnectionImpl.java ---
@@ -132,10 +132,12 @@ protected DrillConnectionImpl(DriverImpl driver, AvaticaFactory factory,
bit = new Drillbit(dConfig, serviceSet);
bit.run();
} catch (final UserException e) {
+ cleanup();
throw new SQLException(
"Failure in starting embedded Drillbit: " + e.getMessage(),
e);
} catch (Exception e) {
+ cleanup();
--- End diff --
I suppose you can use finally block to perform clean up instead of repeating `cleanup` method call after each cached exception.
---
[GitHub] drill pull request #1094: DRILL-6090: While connecting to drill-bits using J...
Posted by milindt <gi...@git.apache.org>.
Github user milindt commented on a diff in the pull request:
https://github.com/apache/drill/pull/1094#discussion_r162286759
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillConnectionImpl.java ---
@@ -132,10 +132,12 @@ protected DrillConnectionImpl(DriverImpl driver, AvaticaFactory factory,
bit = new Drillbit(dConfig, serviceSet);
bit.run();
} catch (final UserException e) {
+ cleanup();
throw new SQLException(
"Failure in starting embedded Drillbit: " + e.getMessage(),
e);
} catch (Exception e) {
+ cleanup();
--- End diff --
We don't want clean up to run on successful connection though, we just want to cover all the exceptions. On a successful connection, the "Curator" threads will be created but the number of threads will be proportional to number of connections, which can be be controlled by using a connection pool, hence it won't cause this issue.
On the other hand, calling cleanup on a successful connection may terminate that connection. Thoughts?
---