[kudu-CR] subprocess: allow Call() to read both stdout and stderr

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] subprocess: allow Call() to read both stdout and stderr

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] subprocess: allow Call() to read both stdout and stderr

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] subprocess: allow Call() to read both stdout and stderr

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] subprocess: allow Call() to read both stdout and stderr

[Kudu Jenkins (Code Review)]

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 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] subprocess: allow Call() to read both stdout and stderr

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Todd Lipcon