You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by sudheeshkatkam <gi...@git.apache.org> on 2016/04/05 07:00:05 UTC
[GitHub] drill pull request: DRILL-3743: Fail active queries if server conn...
GitHub user sudheeshkatkam opened a pull request:
https://github.com/apache/drill/pull/460
DRILL-3743: Fail active queries if server connection is closed
+ Remove dead code
+ Improve error and logging messages
@jacques-n @adeneche please review.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sudheeshkatkam/drill DRILL-3743
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/460.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 #460
----
commit 2c6d156d7e9285abb915941942f2c6e9642b4511
Author: Sudheesh Katkam <sk...@maprtech.com>
Date: 2016-04-05T04:13:10Z
DRILL-3743: Fail active queries if server connection is closed
+ Remove dead code
+ Improve error and logging messages
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3743: Fail active listeners if server co...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on the pull request:
https://github.com/apache/drill/pull/460#issuecomment-206027992
Addressed review comments.
+ Move new changes
+ Improve error and logging messages
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3743: Fail active listeners if server co...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on the pull request:
https://github.com/apache/drill/pull/460#issuecomment-206115245
+1, LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3743: Fail active listeners if server co...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/460
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3743: Fail active listeners if server co...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/460#discussion_r58491456
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java ---
@@ -189,20 +218,10 @@ private UserResultsListener newUserResultsListener(QueryId queryId) {
if ( null == resultsListener ) {
resultsListener = bl;
}
- // TODO: Is there a more direct way to detect a Query ID in whatever state this string comparison detects?
- if ( queryId.toString().isEmpty() ) {
--- End diff --
You removed this because it's impossible to receive a batch without a queryId, right ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3743: Fail active listeners if server co...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/460#discussion_r58613516
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java ---
@@ -278,49 +297,31 @@ public void queryIdArrived(QueryId queryId) {
private class SubmissionListener extends BaseRpcOutcomeListener<QueryId> {
private final UserResultsListener resultsListener;
- private final RemoteConnection connection;
- private final ChannelFuture closeFuture;
- private final ChannelClosedListener closeListener;
private final AtomicBoolean isTerminal = new AtomicBoolean(false);
- public SubmissionListener(RemoteConnection connection, UserResultsListener resultsListener) {
- super();
+ public SubmissionListener(UserResultsListener resultsListener) {
this.resultsListener = resultsListener;
- this.connection = connection;
- this.closeFuture = connection.getChannel().closeFuture();
- this.closeListener = new ChannelClosedListener();
- closeFuture.addListener(closeListener);
- }
-
- private class ChannelClosedListener implements GenericFutureListener<Future<Void>> {
-
- @Override
- public void operationComplete(Future<Void> future) throws Exception {
- resultsListener.submissionFailed(UserException.connectionError()
- .message("Connection %s closed unexpectedly.", connection.getName())
- .build(logger));
- }
-
}
@Override
public void failed(RpcException ex) {
if (!isTerminal.compareAndSet(false, true)) {
+ logger.warn("Received multiple outcomes (success/failure) while submitting query.");
return;
}
- closeFuture.removeListener(closeListener);
- resultsListener.submissionFailed(UserException.systemError(ex).build(logger));
-
+ resultsListener.submissionFailed(UserException.systemError(ex)
+ .message("Unexpected failure while submitting query to a Drillbit.")
--- End diff --
Looks like system errors preserve the original message. So this message was being ignored. I'll add the message to context.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3743: Fail active listeners if server co...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/460#discussion_r58587852
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java ---
@@ -351,20 +352,5 @@ public void success(QueryId queryId, ByteBuf buf) {
}
}
}
-
- @Override
- public void interrupted(final InterruptedException ex) {
--- End diff --
Submissions are asynchronous; no thread is waiting.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3743: Fail active listeners if server co...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/460#discussion_r58491998
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java ---
@@ -351,20 +352,5 @@ public void success(QueryId queryId, ByteBuf buf) {
}
}
}
-
- @Override
- public void interrupted(final InterruptedException ex) {
--- End diff --
why remove this ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3743: Fail active listeners if server co...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/460#discussion_r58589485
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java ---
@@ -351,20 +352,5 @@ public void success(QueryId queryId, ByteBuf buf) {
}
}
}
-
- @Override
- public void interrupted(final InterruptedException ex) {
--- End diff --
how about [this](https://github.com/apache/drill/blob/master/exec/rpc/src/main/java/org/apache/drill/exec/rpc/RemoteConnection.java#L74) ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3743: Fail active listeners if server co...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/460#discussion_r58491800
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java ---
@@ -278,49 +297,31 @@ public void queryIdArrived(QueryId queryId) {
private class SubmissionListener extends BaseRpcOutcomeListener<QueryId> {
private final UserResultsListener resultsListener;
- private final RemoteConnection connection;
- private final ChannelFuture closeFuture;
- private final ChannelClosedListener closeListener;
private final AtomicBoolean isTerminal = new AtomicBoolean(false);
- public SubmissionListener(RemoteConnection connection, UserResultsListener resultsListener) {
- super();
+ public SubmissionListener(UserResultsListener resultsListener) {
this.resultsListener = resultsListener;
- this.connection = connection;
- this.closeFuture = connection.getChannel().closeFuture();
- this.closeListener = new ChannelClosedListener();
- closeFuture.addListener(closeListener);
- }
-
- private class ChannelClosedListener implements GenericFutureListener<Future<Void>> {
-
- @Override
- public void operationComplete(Future<Void> future) throws Exception {
- resultsListener.submissionFailed(UserException.connectionError()
- .message("Connection %s closed unexpectedly.", connection.getName())
- .build(logger));
- }
-
}
@Override
public void failed(RpcException ex) {
if (!isTerminal.compareAndSet(false, true)) {
+ logger.warn("Received multiple outcomes (success/failure) while submitting query.");
return;
}
- closeFuture.removeListener(closeListener);
- resultsListener.submissionFailed(UserException.systemError(ex).build(logger));
-
+ resultsListener.submissionFailed(UserException.systemError(ex)
+ .message("Unexpected failure while submitting query to a Drillbit.")
--- End diff --
This will override the original exception's message. You should either include it in the error message, or if you think it's not useful to the end user, log it so dev can find what wrong
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3743: Fail active listeners if server co...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/460#discussion_r58595373
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java ---
@@ -351,20 +352,5 @@ public void success(QueryId queryId, ByteBuf buf) {
}
}
}
-
- @Override
- public void interrupted(final InterruptedException ex) {
--- End diff --
My bad, I did not consider back pressure. The warning message is misleading. I'll update the warning and user exception message.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3743: Fail active listeners if server co...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/460#discussion_r58587275
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java ---
@@ -278,49 +297,31 @@ public void queryIdArrived(QueryId queryId) {
private class SubmissionListener extends BaseRpcOutcomeListener<QueryId> {
private final UserResultsListener resultsListener;
- private final RemoteConnection connection;
- private final ChannelFuture closeFuture;
- private final ChannelClosedListener closeListener;
private final AtomicBoolean isTerminal = new AtomicBoolean(false);
- public SubmissionListener(RemoteConnection connection, UserResultsListener resultsListener) {
- super();
+ public SubmissionListener(UserResultsListener resultsListener) {
this.resultsListener = resultsListener;
- this.connection = connection;
- this.closeFuture = connection.getChannel().closeFuture();
- this.closeListener = new ChannelClosedListener();
- closeFuture.addListener(closeListener);
- }
-
- private class ChannelClosedListener implements GenericFutureListener<Future<Void>> {
-
- @Override
- public void operationComplete(Future<Void> future) throws Exception {
- resultsListener.submissionFailed(UserException.connectionError()
- .message("Connection %s closed unexpectedly.", connection.getName())
- .build(logger));
- }
-
}
@Override
public void failed(RpcException ex) {
if (!isTerminal.compareAndSet(false, true)) {
+ logger.warn("Received multiple outcomes (success/failure) while submitting query.");
return;
}
- closeFuture.removeListener(closeListener);
- resultsListener.submissionFailed(UserException.systemError(ex).build(logger));
-
+ resultsListener.submissionFailed(UserException.systemError(ex)
+ .message("Unexpected failure while submitting query to a Drillbit.")
--- End diff --
will update.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3743: Fail active listeners if server co...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/460#discussion_r58589199
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java ---
@@ -189,20 +218,10 @@ private UserResultsListener newUserResultsListener(QueryId queryId) {
if ( null == resultsListener ) {
resultsListener = bl;
}
- // TODO: Is there a more direct way to detect a Query ID in whatever state this string comparison detects?
- if ( queryId.toString().isEmpty() ) {
--- End diff --
Yes; if query id string was empty, there would be failures on the server side (like exchanges).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3743: Fail active listeners if server co...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/460#discussion_r58650141
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java ---
@@ -354,17 +335,57 @@ public void success(QueryId queryId, ByteBuf buf) {
@Override
public void interrupted(final InterruptedException ex) {
- logger.warn("Interrupted while waiting for query results from Drillbit", ex);
-
if (!isTerminal.compareAndSet(false, true)) {
+ logger.warn("Received multiple responses to run query request.");
return;
}
- closeFuture.removeListener(closeListener);
-
// Throw an interrupted UserException?
- resultsListener.submissionFailed(UserException.systemError(ex).build(logger));
+ resultsListener.submissionFailed(UserException.systemError(ex)
--- End diff --
We shouldn't use a "system error" when we want to display a proper error message. System errors are meant for unexpected errors that don't have a "nice" error message yet.
This may require adding a new "client" user exception type, to make it easier to differentiate errors that happen on the client side.
This is may be too much of a change for this pull request, so it's fine if you just create a JIRA to address this change later, as I think there are other places in the client code that could also throw a client error instead of a system error.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---