You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Attila Bukor (Code Review)" <ge...@cloudera.org> on 2020/03/31 10:48:27 UTC
[kudu-CR] [subprocess] Fix shutdown behavior
Hello Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/15615
to review the following change.
Change subject: [subprocess] Fix shutdown behavior
......................................................................
[subprocess] Fix shutdown behavior
Prior to this commit the subprocess didn't actually shut down when its
input stream was closed. This wasn't much of a problem on Linux where
PDEATHSIG is set on the child process, but as it's not supported on Mac,
whenever the master is shut down/crashes the subprocess stays alive.
This commit changes the behavior in the subprocess to wrap the
IOException in KuduSubprocessException instead of simply breaking the
loop (in which case the other threads would keep running uninterrupted),
and the futures' exception handler exits the program.
Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
---
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java
2 files changed, 3 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/15615/1
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
[kudu-CR] [subprocess] Fix shutdown behavior
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15615
to look at the new patch set (#4).
Change subject: [subprocess] Fix shutdown behavior
......................................................................
[subprocess] Fix shutdown behavior
Prior to this commit the subprocess didn't actually shut down when its
input stream was closed. This wasn't much of a problem on Linux where
PDEATHSIG is set on the child process, but as it's not supported on Mac,
whenever the master is shut down/crashes the subprocess stays alive.
This commit changes the behavior to exit when the reader future exits
(the loop is broken when EOF is received).
Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
---
M java/config/spotbugs/excludeFilter.xml
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java
2 files changed, 10 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/15615/4
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [subprocess] Fix shutdown behavior
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15615 )
Change subject: [subprocess] Fix shutdown behavior
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 01 Apr 2020 21:47:52 +0000
Gerrit-HasComments: No
[kudu-CR] [subprocess] Fix shutdown behavior
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15615 )
Change subject: [subprocess] Fix shutdown behavior
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15615/3/java/config/spotbugs/excludeFilter.xml
File java/config/spotbugs/excludeFilter.xml:
http://gerrit.cloudera.org:8080/#/c/15615/3/java/config/spotbugs/excludeFilter.xml@311
PS3, Line 311: all threads stop --
> FWIW I think the existing alignment was consistent with the existing file.
seems the file itself is pretty inconsistent, for example in L39-40 there's a space
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 01 Apr 2020 19:39:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] [subprocess] Fix shutdown behavior
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15615 )
Change subject: [subprocess] Fix shutdown behavior
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15615/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java
File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java:
http://gerrit.cloudera.org:8080/#/c/15615/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@68
PS1, Line 68: System.exit(1);
> If a RuntimeException is thrown and isn't caught, that thread will be stopp
If we're calling System.exit(), we don't need to throw a new exception, right? That's effectively dead code.
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 31 Mar 2020 20:22:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] [subprocess] Fix shutdown behavior
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15615 )
Change subject: [subprocess] Fix shutdown behavior
......................................................................
Patch Set 3: Verified+1
Nothing to add upon Andrew's comment.
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 01 Apr 2020 00:21:22 +0000
Gerrit-HasComments: No
[kudu-CR] [subprocess] Fix shutdown behavior
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15615
to look at the new patch set (#6).
Change subject: [subprocess] Fix shutdown behavior
......................................................................
[subprocess] Fix shutdown behavior
Prior to this commit the subprocess didn't actually shut down when its
input stream was closed. This wasn't much of a problem on Linux where
PDEATHSIG is set on the child process, but as it's not supported on Mac,
whenever the master is shut down/crashes the subprocess stays alive.
This commit changes the behavior to exit when the reader future exits
(the loop is broken when EOF is received).
Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
---
M java/config/spotbugs/excludeFilter.xml
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java
2 files changed, 12 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/15615/6
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [subprocess] Fix shutdown behavior
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15615 )
Change subject: [subprocess] Fix shutdown behavior
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15615/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java
File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java:
http://gerrit.cloudera.org:8080/#/c/15615/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java@71
PS1, Line 71: // Throw a subprocess exception if EOF is reached.
> nit: update the comment.
Done
http://gerrit.cloudera.org:8080/#/c/15615/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java@72
PS1, Line 72: throw new KuduSubprocessException("Stream closed", e);
> So with the current change, if MessageReader reaches the end of the stream
yes
http://gerrit.cloudera.org:8080/#/c/15615/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java
File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java:
http://gerrit.cloudera.org:8080/#/c/15615/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@68
PS1, Line 68: System.exit(1);
> Why do we need this as we are throwing exception below?
If a RuntimeException is thrown and isn't caught, that thread will be stopped, but the others will still keep running, so in this case the program would still continue running even if we stop one thread.
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 31 Mar 2020 19:43:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] [subprocess] Fix shutdown behavior
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15615 )
Change subject: [subprocess] Fix shutdown behavior
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15615/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java
File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java:
http://gerrit.cloudera.org:8080/#/c/15615/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java@71
PS1, Line 71: // Break the loop if the end of the stream has been reached.
nit: update the comment.
http://gerrit.cloudera.org:8080/#/c/15615/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java@72
PS1, Line 72: throw new KuduSubprocessException("Stream closed", e);
So with the current change, if MessageReader reaches the end of the stream and there is still some unprocessed message in MessageParser(MessageWriter), the master will not able to get these responses back, but that shouldn't be an issue as the master should already close the pipe, right?
http://gerrit.cloudera.org:8080/#/c/15615/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java
File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java:
http://gerrit.cloudera.org:8080/#/c/15615/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@68
PS1, Line 68: System.exit(1);
Why do we need this as we are throwing exception below?
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 31 Mar 2020 18:35:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] [subprocess] Fix shutdown behavior
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15615
to look at the new patch set (#2).
Change subject: [subprocess] Fix shutdown behavior
......................................................................
[subprocess] Fix shutdown behavior
Prior to this commit the subprocess didn't actually shut down when its
input stream was closed. This wasn't much of a problem on Linux where
PDEATHSIG is set on the child process, but as it's not supported on Mac,
whenever the master is shut down/crashes the subprocess stays alive.
This commit changes the behavior in the subprocess to wrap the
IOException in KuduSubprocessException instead of simply breaking the
loop (in which case the other threads would keep running uninterrupted),
and the futures' exception handler exits the program.
Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
---
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java
2 files changed, 4 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/15615/2
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [subprocess] Fix shutdown behavior
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15615 )
Change subject: [subprocess] Fix shutdown behavior
......................................................................
Patch Set 8: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 8
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 02 Apr 2020 16:51:14 +0000
Gerrit-HasComments: No
[kudu-CR] [subprocess] Fix shutdown behavior
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15615 )
Change subject: [subprocess] Fix shutdown behavior
......................................................................
Patch Set 5: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15615/3/java/config/spotbugs/excludeFilter.xml
File java/config/spotbugs/excludeFilter.xml:
http://gerrit.cloudera.org:8080/#/c/15615/3/java/config/spotbugs/excludeFilter.xml@311
PS3, Line 311: all threads stop --
> Done
FWIW I think the existing alignment was consistent with the existing file. See L220 for example.
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 01 Apr 2020 19:14:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] [subprocess] Fix shutdown behavior
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15615 )
Change subject: [subprocess] Fix shutdown behavior
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15615/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java
File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java:
http://gerrit.cloudera.org:8080/#/c/15615/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@116
PS5, Line 116: // TODO(abukor): Refactor code so the futures can be cance
> Can you comment that we are forcing the program to exit when the reader fut
Done
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 02 Apr 2020 09:24:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] [subprocess] Fix shutdown behavior
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15615 )
Change subject: [subprocess] Fix shutdown behavior
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 31 Mar 2020 22:57:04 +0000
Gerrit-HasComments: No
[kudu-CR] [subprocess] Fix shutdown behavior
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15615
to look at the new patch set (#3).
Change subject: [subprocess] Fix shutdown behavior
......................................................................
[subprocess] Fix shutdown behavior
Prior to this commit the subprocess didn't actually shut down when its
input stream was closed. This wasn't much of a problem on Linux where
PDEATHSIG is set on the child process, but as it's not supported on Mac,
whenever the master is shut down/crashes the subprocess stays alive.
This commit changes the behavior in the subprocess to wrap the
IOException in KuduSubprocessException instead of simply breaking the
loop (in which case the other threads would keep running uninterrupted),
and the futures' exception handler exits the program.
Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
---
M java/config/spotbugs/excludeFilter.xml
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java
3 files changed, 12 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/15615/3
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [subprocess] Fix shutdown behavior
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15615 )
Change subject: [subprocess] Fix shutdown behavior
......................................................................
[subprocess] Fix shutdown behavior
Prior to this commit the subprocess didn't actually shut down when its
input stream was closed. This wasn't much of a problem on Linux where
PDEATHSIG is set on the child process, but as it's not supported on Mac,
whenever the master is shut down/crashes the subprocess stays alive.
This commit changes the behavior to exit when the reader future exits
(the loop is broken when EOF is received).
Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Reviewed-on: http://gerrit.cloudera.org:8080/15615
Tested-by: Kudu Jenkins
Reviewed-by: Hao Hao <ha...@cloudera.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M java/config/spotbugs/excludeFilter.xml
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java
2 files changed, 12 insertions(+), 1 deletion(-)
Approvals:
Kudu Jenkins: Verified
Hao Hao: Looks good to me, approved
Adar Dembo: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 9
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [subprocess] Fix shutdown behavior
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15615
to look at the new patch set (#5).
Change subject: [subprocess] Fix shutdown behavior
......................................................................
[subprocess] Fix shutdown behavior
Prior to this commit the subprocess didn't actually shut down when its
input stream was closed. This wasn't much of a problem on Linux where
PDEATHSIG is set on the child process, but as it's not supported on Mac,
whenever the master is shut down/crashes the subprocess stays alive.
This commit changes the behavior to exit when the reader future exits
(the loop is broken when EOF is received).
Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
---
M java/config/spotbugs/excludeFilter.xml
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java
2 files changed, 10 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/15615/5
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [subprocess] Fix shutdown behavior
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15615 )
Change subject: [subprocess] Fix shutdown behavior
......................................................................
Patch Set 3: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15615/3/java/config/spotbugs/excludeFilter.xml
File java/config/spotbugs/excludeFilter.xml:
http://gerrit.cloudera.org:8080/#/c/15615/3/java/config/spotbugs/excludeFilter.xml@311
PS3, Line 311: all threads stop -->
nit: add a space to conform to the existing style here
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 31 Mar 2020 22:56:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] [subprocess] Fix shutdown behavior
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15615 )
Change subject: [subprocess] Fix shutdown behavior
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15615/3/java/config/spotbugs/excludeFilter.xml
File java/config/spotbugs/excludeFilter.xml:
http://gerrit.cloudera.org:8080/#/c/15615/3/java/config/spotbugs/excludeFilter.xml@311
PS3, Line 311: all threads stop --
> nit: add a space to conform to the existing style here
Done
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 01 Apr 2020 09:25:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] [subprocess] Fix shutdown behavior
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15615 )
Change subject: [subprocess] Fix shutdown behavior
......................................................................
Patch Set 8: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 8
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 02 Apr 2020 21:23:16 +0000
Gerrit-HasComments: No
[kudu-CR] [subprocess] Fix shutdown behavior
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15615 )
Change subject: [subprocess] Fix shutdown behavior
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15615/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java
File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java:
http://gerrit.cloudera.org:8080/#/c/15615/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@116
PS5, Line 116: readerFuture = readerFuture.thenRun(() -> System.exit(0));
Can you comment that we are forcing the program to exit when the reader future finishes execution?
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 01 Apr 2020 21:39:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] [subprocess] Fix shutdown behavior
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15615 )
Change subject: [subprocess] Fix shutdown behavior
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15615/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java
File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java:
http://gerrit.cloudera.org:8080/#/c/15615/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@68
PS1, Line 68: System.exit(1);
> If we're calling System.exit(), we don't need to throw a new exception, rig
yep, but it seems Java still expects return or throw after it, I replaced it with return null though, I think that's less confusing.
--
To view, visit http://gerrit.cloudera.org:8080/15615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia84bff891eaff667d7b22e89c66f24980ad76d70
Gerrit-Change-Number: 15615
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 31 Mar 2020 21:34:41 +0000
Gerrit-HasComments: Yes