You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2016/08/19 04:07:04 UTC
[kudu-CR] subprocess: allow Call() to read both stdout and stderr
Hello Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4057
to review the following change.
Change subject: subprocess: allow Call() to read both stdout and stderr
......................................................................
subprocess: allow Call() to read both stdout and stderr
I'm going to use this in a new integration test for the tool.
Since the parent is now reading from two pipes, it needs to do so more
carefully. For example, if it read from stdout fully before looking at
stderr, both it and the child would deadlock if the child wrote 64k bytes
to the stderr pipe, hit the kernel limit, and got blocked.
I played around with an implementation based on poll(), but ultimately found
this one (based on libev) to be simpler.
Change-Id: If5f2be94c2e5cc0644a5bb2340adc4a71d844247
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
4 files changed, 163 insertions(+), 64 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/4057/1
--
To view, visit http://gerrit.cloudera.org:8080/4057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: If5f2be94c2e5cc0644a5bb2340adc4a71d844247
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] subprocess: allow Call() to read both stdout and stderr
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: subprocess: allow Call() to read both stdout and stderr
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4057/1/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:
Line 187: // Interrupted by a signal, do nothing.
> I think so; returning here just means we'll poll the fd again, which will i
ah, OK, I was thinking that libev was edge-triggered rather than level-triggered, but I remembered wrong: http://lists.schmorp.de/pipermail/libev/2010q4/001162.html
--
To view, visit http://gerrit.cloudera.org:8080/4057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If5f2be94c2e5cc0644a5bb2340adc4a71d844247
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] subprocess: allow Call() to read both stdout and stderr
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: subprocess: allow Call() to read both stdout and stderr
......................................................................
Patch Set 1: Code-Review+1
Test passes on OS X
--
To view, visit http://gerrit.cloudera.org:8080/4057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If5f2be94c2e5cc0644a5bb2340adc4a71d844247
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] subprocess: allow Call() to read both stdout and stderr
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: subprocess: allow Call() to read both stdout and stderr
......................................................................
subprocess: allow Call() to read both stdout and stderr
I'm going to use this in a new integration test for the tool.
Since the parent is now reading from two pipes, it needs to do so more
carefully. For example, if it read from stdout fully before looking at
stderr, both it and the child would deadlock if the child wrote 64k bytes
to the stderr pipe, hit the kernel limit, and got blocked.
I played around with an implementation based on poll(), but ultimately found
this one (based on libev) to be simpler.
Change-Id: If5f2be94c2e5cc0644a5bb2340adc4a71d844247
Reviewed-on: http://gerrit.cloudera.org:8080/4057
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
4 files changed, 163 insertions(+), 64 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: If5f2be94c2e5cc0644a5bb2340adc4a71d844247
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] subprocess: allow Call() to read both stdout and stderr
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: subprocess: allow Call() to read both stdout and stderr
......................................................................
Patch Set 1:
Build Started http://104.196.14.100/job/kudu-gerrit/2986/
--
To view, visit http://gerrit.cloudera.org:8080/4057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If5f2be94c2e5cc0644a5bb2340adc4a71d844247
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] subprocess: allow Call() to read both stdout and stderr
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: subprocess: allow Call() to read both stdout and stderr
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4057/1/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:
Line 187: // Interrupted by a signal, do nothing.
is this the appropriate thing instead of retrying on EINTR? (like RETRY_ON_EINTR macro in env_posix.cc)
Line 487: *stdout_out = out[0];
maybe std::move or a swap() makes sense here just cuz the output could be large
--
To view, visit http://gerrit.cloudera.org:8080/4057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If5f2be94c2e5cc0644a5bb2340adc4a71d844247
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] subprocess: allow Call() to read both stdout and stderr
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: subprocess: allow Call() to read both stdout and stderr
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If5f2be94c2e5cc0644a5bb2340adc4a71d844247
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] subprocess: allow Call() to read both stdout and stderr
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4057
to look at the new patch set (#2).
Change subject: subprocess: allow Call() to read both stdout and stderr
......................................................................
subprocess: allow Call() to read both stdout and stderr
I'm going to use this in a new integration test for the tool.
Since the parent is now reading from two pipes, it needs to do so more
carefully. For example, if it read from stdout fully before looking at
stderr, both it and the child would deadlock if the child wrote 64k bytes
to the stderr pipe, hit the kernel limit, and got blocked.
I played around with an implementation based on poll(), but ultimately found
this one (based on libev) to be simpler.
Change-Id: If5f2be94c2e5cc0644a5bb2340adc4a71d844247
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
4 files changed, 163 insertions(+), 64 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/4057/2
--
To view, visit http://gerrit.cloudera.org:8080/4057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If5f2be94c2e5cc0644a5bb2340adc4a71d844247
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] subprocess: allow Call() to read both stdout and stderr
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: subprocess: allow Call() to read both stdout and stderr
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4057/1/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:
Line 187: // Interrupted by a signal, do nothing.
> is this the appropriate thing instead of retrying on EINTR? (like RETRY_ON_
I think so; returning here just means we'll poll the fd again, which will indicate that there's more data to be read, and we'll end up right back here on the next iteration.
Line 487: *stdout_out = out[0];
> maybe std::move or a swap() makes sense here just cuz the output could be l
Done
--
To view, visit http://gerrit.cloudera.org:8080/4057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If5f2be94c2e5cc0644a5bb2340adc4a71d844247
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] subprocess: allow Call() to read both stdout and stderr
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: subprocess: allow Call() to read both stdout and stderr
......................................................................
Patch Set 2:
Build Started http://104.196.14.100/job/kudu-gerrit/3059/
--
To view, visit http://gerrit.cloudera.org:8080/4057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If5f2be94c2e5cc0644a5bb2340adc4a71d844247
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No