You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by arina-ielchiieva <gi...@git.apache.org> on 2017/06/20 11:42:57 UTC
[GitHub] drill pull request #857: DRILL-5599: Notify StatusHandlerListener that batch...
GitHub user arina-ielchiieva opened a pull request:
https://github.com/apache/drill/pull/857
DRILL-5599: Notify StatusHandlerListener that batch sending has failed even if channel is still open
Details in [DRILL-5599](https://issues.apache.org/jira/browse/DRILL-5599)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/arina-ielchiieva/drill DRILL-5599
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/857.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 #857
----
commit 4cf04b6476de899bea6e6cfab98f3e8e2aaaa7c6
Author: Arina Ielchiieva <ar...@gmail.com>
Date: 2017-06-20T09:18:27Z
DRILL-5599: Notify StatusHandlerListener that batch sending has failed even if channel is still open
----
---
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 #857: DRILL-5599: Notify StatusHandler that batch sending...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on a diff in the pull request:
https://github.com/apache/drill/pull/857#discussion_r123118252
--- Diff: exec/rpc/src/main/java/org/apache/drill/exec/rpc/RequestIdMap.java ---
@@ -111,13 +111,16 @@ public RpcListener(RpcOutcomeListener<T> handler, Class<T> clazz, int coordinati
@Override
public void operationComplete(ChannelFuture future) throws Exception {
-
if (!future.isSuccess()) {
- removeFromMap(coordinationId);
- if (future.channel().isActive()) {
- throw new RpcException("Future failed");
- } else {
- setException(new ChannelClosedException());
+ try {
+ removeFromMap(coordinationId);
+ } finally {
+ final Throwable cause = future.cause();
+ if (future.channel().isActive()) {
+ setException(cause == null ? new RpcException("Future has failed") : cause);
--- End diff --
Can we pick a better message ? "Future has failed" does not convey anything useful to the end user.
---
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 #857: DRILL-5599: Notify StatusHandler that batch sending...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on a diff in the pull request:
https://github.com/apache/drill/pull/857#discussion_r123118364
--- Diff: exec/rpc/src/main/java/org/apache/drill/exec/rpc/RequestIdMap.java ---
@@ -47,7 +47,7 @@
private final IntObjectHashMap<RpcOutcome<?>> map;
public RequestIdMap() {
- map = new IntObjectHashMap<RpcOutcome<?>>();
+ map = new IntObjectHashMap<>();
}
--- End diff --
why do we need this change ?
---
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 #857: DRILL-5599: Notify StatusHandler that batch sending...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on a diff in the pull request:
https://github.com/apache/drill/pull/857#discussion_r123118764
--- Diff: exec/rpc/src/main/java/org/apache/drill/exec/rpc/RequestIdMap.java ---
@@ -111,13 +111,16 @@ public RpcListener(RpcOutcomeListener<T> handler, Class<T> clazz, int coordinati
@Override
public void operationComplete(ChannelFuture future) throws Exception {
-
if (!future.isSuccess()) {
- removeFromMap(coordinationId);
- if (future.channel().isActive()) {
- throw new RpcException("Future failed");
- } else {
- setException(new ChannelClosedException());
+ try {
+ removeFromMap(coordinationId);
+ } finally {
+ final Throwable cause = future.cause();
+ if (future.channel().isActive()) {
+ setException(cause == null ? new RpcException("Future has failed") : cause);
--- End diff --
overall, LGTM. Some minor comments.
---
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 #857: DRILL-5599: Notify StatusHandler that batch sending...
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/857#discussion_r123135163
--- Diff: exec/rpc/src/main/java/org/apache/drill/exec/rpc/RequestIdMap.java ---
@@ -47,7 +47,7 @@
private final IntObjectHashMap<RpcOutcome<?>> map;
public RequestIdMap() {
- map = new IntObjectHashMap<RpcOutcome<?>>();
+ map = new IntObjectHashMap<>();
}
--- End diff --
This is code cleanup. Java can infer the type of the parameterized class without us telling it. Seems a similar cleanup was made a few lines below.
---
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 #857: DRILL-5599: Notify StatusHandler that batch sending...
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/857#discussion_r123135561
--- Diff: exec/rpc/src/main/java/org/apache/drill/exec/rpc/RequestIdMap.java ---
@@ -111,13 +111,16 @@ public RpcListener(RpcOutcomeListener<T> handler, Class<T> clazz, int coordinati
@Override
public void operationComplete(ChannelFuture future) throws Exception {
-
if (!future.isSuccess()) {
- removeFromMap(coordinationId);
- if (future.channel().isActive()) {
- throw new RpcException("Future failed");
- } else {
- setException(new ChannelClosedException());
+ try {
+ removeFromMap(coordinationId);
+ } finally {
+ final Throwable cause = future.cause();
+ if (future.channel().isActive()) {
+ setException(cause == null ? new RpcException("Future has failed") : cause);
--- End diff --
I think the point is that the message is not sent to the end user, or written to the log. Rather, it is letting the listener know that the send failed. There is a reason for the failure. Ideally the `cause` is the real reason.
Still it would be good if we were to log such failures. So, maybe "Unknown channel failure" or some such might be helpful. Probably the developer will search the code to find where we raise that exception, so the message just has to be unique...
---
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 #857: DRILL-5599: Notify StatusHandler that batch sending...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/857
---
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 issue #857: DRILL-5599: Notify StatusHandler that batch sending has fa...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/857
Reworded error message. @paul-rogers and @ppadma thanks for code review!
---
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.
---