You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by vrozov <gi...@git.apache.org> on 2017/09/07 02:02:17 UTC
[GitHub] drill pull request #934: DRILL-3449 When Foreman node dies, the FragmentExec...
GitHub user vrozov opened a pull request:
https://github.com/apache/drill/pull/934
DRILL-3449 When Foreman node dies, the FragmentExecutor still tries to send status updates to Foreman
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vrozov/drill DRILL-3449
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/934.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 #934
----
commit 06f15eeaf34c610fce0e3a62ae635c10f9885608
Author: Vlad Rozov <vr...@apache.org>
Date: 2017-09-07T01:29:02Z
DRILL-3449 When Foreman node dies, the FragmentExecutor still tries to send status updates to Foreman
----
---
[GitHub] drill pull request #934: DRILL-3449 When Foreman node dies, the FragmentExec...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/934#discussion_r137941417
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentStatusReporter.java ---
@@ -113,4 +120,14 @@ void fail(final UserException ex) {
sendStatus(status);
}
+ @Override
+ public void close()
+ {
+ final ControlTunnel tunnel = this.tunnel.getAndSet(null);
+ if (tunnel != null) {
+ logger.debug("Closing {}", this);
--- End diff --
No, close() is not a placeholder. It closes FragmentStatusReporter and after the close, request to send status becomes no-op.
---
[GitHub] drill pull request #934: DRILL-3449 When Foreman node dies, the FragmentExec...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/934
---
[GitHub] drill issue #934: DRILL-3449 When Foreman node dies, the FragmentExecutor st...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/934
@paul-rogers Please review
---
[GitHub] drill pull request #934: DRILL-3449 When Foreman node dies, the FragmentExec...
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/934#discussion_r137938397
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentStatusReporter.java ---
@@ -113,4 +120,14 @@ void fail(final UserException ex) {
sendStatus(status);
}
+ @Override
+ public void close()
+ {
+ final ControlTunnel tunnel = this.tunnel.getAndSet(null);
+ if (tunnel != null) {
+ logger.debug("Closing {}", this);
--- End diff --
We say we are closing the tunnel, but then don't do anything. Perhaps a comment to explain how this works?
---
[GitHub] drill issue #934: DRILL-3449 When Foreman node dies, the FragmentExecutor st...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/934
Thanks for the explanation.
+1
---
[GitHub] drill pull request #934: DRILL-3449 When Foreman node dies, the FragmentExec...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/934#discussion_r137941124
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentStatusReporter.java ---
@@ -113,4 +120,14 @@ void fail(final UserException ex) {
sendStatus(status);
}
+ @Override
+ public void close()
+ {
+ final ControlTunnel tunnel = this.tunnel.getAndSet(null);
+ if (tunnel != null) {
+ logger.debug("Closing {}", this);
--- End diff --
We are closing FragmentStatusReporter, not the `tunnel` that it references. The ControlTunnel is not Closable even though it has a reference to a resource that is Closable and should provide a way to release the resource it holds. Please let me know if a comment is required here, but I do plan to make ControlTunnel Closable. As it requires code refactoring not directly related to the JIRA/PR, I plan to do this in a separate PR.
---