[kudu-CR] [util] minor clean-up on kudu::Subprocess

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [util] minor clean-up on kudu::Subprocess
..


Patch Set 5:

> I'm going to push even though Tidy Bot flagged another warning. It
 > did so because you modified the nearby area, but I don't mind
 > ignoring it since the warning is to lines you didn't modify.

Great.  Thank you Adar!

-- 
To view, visit http://gerrit.cloudera.org:8080/4502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idd058382e4519b323aebb4c992d9088496a341cc
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [util] minor clean-up on kudu::Subprocess

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged.

Change subject: [util] minor clean-up on kudu::Subprocess
..


[util] minor clean-up on kudu::Subprocess

Do not call CHECK_EQ() in case where it's possible to report
on error via the return value.

Change-Id: Idd058382e4519b323aebb4c992d9088496a341cc
Reviewed-on: http://gerrit.cloudera.org:8080/4502
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M src/kudu/benchmarks/tpch/tpch_real_world.cc
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/util/subprocess.cc
3 files changed, 42 insertions(+), 19 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/4502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Idd058382e4519b323aebb4c992d9088496a341cc
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [util] minor clean-up on kudu::Subprocess

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [util] minor clean-up on kudu::Subprocess
..


Patch Set 4: Code-Review+2

I'm going to push even though Tidy Bot flagged another warning. It did so 
because you modified the nearby area, but I don't mind ignoring it since the 
warning is to lines you didn't modify.

-- 
To view, visit http://gerrit.cloudera.org:8080/4502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idd058382e4519b323aebb4c992d9088496a341cc
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [util] minor clean-up on kudu::Subprocess

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4502

to look at the new patch set (#4).

Change subject: [util] minor clean-up on kudu::Subprocess
..

[util] minor clean-up on kudu::Subprocess

Do not call CHECK_EQ() in case where it's possible to report
on error via the return value.

Change-Id: Idd058382e4519b323aebb4c992d9088496a341cc
---
M src/kudu/benchmarks/tpch/tpch_real_world.cc
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/util/subprocess.cc
3 files changed, 42 insertions(+), 19 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/4502/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idd058382e4519b323aebb4c992d9088496a341cc
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [util] minor clean-up on kudu::Subprocess

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4502

to look at the new patch set (#3).

Change subject: [util] minor clean-up on kudu::Subprocess
..

[util] minor clean-up on kudu::Subprocess

Do not call CHECK_EQ() in case where it's possible to report
on error via the return value.

Change-Id: Idd058382e4519b323aebb4c992d9088496a341cc
---
M src/kudu/benchmarks/tpch/tpch_real_world.cc
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/util/subprocess.cc
3 files changed, 41 insertions(+), 19 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/4502/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idd058382e4519b323aebb4c992d9088496a341cc
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [util] minor clean-up on kudu::Subprocess

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [util] minor clean-up on kudu::Subprocess
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4502/2/src/kudu/benchmarks/tpch/tpch_real_world.cc
File src/kudu/benchmarks/tpch/tpch_real_world.cc:

PS2, Line 303:   Status s = dbgen_proc->Kill(SIGKILL);
 :   if (!s.ok()) {
 : LOG(WARNING) << "Failed to send SIGKILL to dbgen: " << 
s.ToString();
 :   }
 :   s = dbgen_proc->Wait(nullptr);
 :   if (!s.ok()) {
 : LOG(WARNING) << "Failed to await for dbgen exit: " << 
s.ToString();
 :   }
> Why aren't these fatal? That is, why not CHECK_OK(...)?
I left them non-fatal as there were in the original code, just added logs.

But you made a good point here: if dbgen hasn't exited, there might be 
surprises.  That's a good observation and I'll change this to LOG(FATAL).


http://gerrit.cloudera.org:8080/#/c/4502/2/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:

PS2, Line 422: *ret = cached_rc_;
> If you want to allow ret to be nullptr, you should also add a  check here b
Good catch, thanks!  I missed that somehow; will fix.


-- 
To view, visit http://gerrit.cloudera.org:8080/4502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idd058382e4519b323aebb4c992d9088496a341cc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [util] minor clean-up on kudu::Subprocess

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [util] minor clean-up on kudu::Subprocess
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4502/2/src/kudu/benchmarks/tpch/tpch_real_world.cc
File src/kudu/benchmarks/tpch/tpch_real_world.cc:

PS2, Line 303:   Status s = dbgen_proc->Kill(SIGKILL);
 :   if (!s.ok()) {
 : LOG(WARNING) << "Failed to send SIGKILL to dbgen: " << 
s.ToString();
 :   }
 :   s = dbgen_proc->Wait(nullptr);
 :   if (!s.ok()) {
 : LOG(WARNING) << "Failed to await for dbgen exit: " << 
s.ToString();
 :   }
Why aren't these fatal? That is, why not CHECK_OK(...)?


http://gerrit.cloudera.org:8080/#/c/4502/2/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:

PS2, Line 422: *ret = cached_rc_;
If you want to allow ret to be nullptr, you should also add a  check here 
before the assignment.


-- 
To view, visit http://gerrit.cloudera.org:8080/4502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idd058382e4519b323aebb4c992d9088496a341cc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [util] minor clean-up on kudu::Subprocess

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4502

to look at the new patch set (#2).

Change subject: [util] minor clean-up on kudu::Subprocess
..

[util] minor clean-up on kudu::Subprocess

Do not call CHECK_EQ() in case where it's possible to report
on error via the return value.

Change-Id: Idd058382e4519b323aebb4c992d9088496a341cc
---
M src/kudu/benchmarks/tpch/tpch_real_world.cc
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/util/subprocess.cc
3 files changed, 38 insertions(+), 18 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/4502/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idd058382e4519b323aebb4c992d9088496a341cc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [util] minor clean-up on kudu::Subprocess

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [util] minor clean-up on kudu::Subprocess
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4502/1/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:

PS1, Line 353: unique_ptr< DIR, std::function >
> could also use MakeScopedCleanup here
ok, thank you for the suggestion.  Though, I think to keep unique_ptr, if there 
are no strong objections.


PS1, Line 353: unique_ptr< DIR, std::function >
> Nit: remove the spaces separating the triangular brackets from other stuff 
Nope, the compiler could not do that because, as I understand, the signature of 
the constructor cannot override the explicitly specified template spec.   The 
shared_ptr has different signature, btw.


PS1, Line 445:   if (state_ != kRunning) {
 : return Status::IllegalState("Sub-process is not running");
 :   }
> I don't know about this. My impression is that the intent here was to be sy
I think that calling abort() or using other ways to abruptly terminate a 
program is only for cases when it's not possible to handle an error 
consistently both for the caller and the callee.  Here we can report on a error 
related to the illegal state via return code without other side-effects.  Why 
not to do it that way?

I can add DCHECK() here if you think debug assert is needed.


-- 
To view, visit http://gerrit.cloudera.org:8080/4502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idd058382e4519b323aebb4c992d9088496a341cc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [util] minor clean-up on kudu::Subprocess

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [util] minor clean-up on kudu::Subprocess
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4502/1/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:

PS1, Line 353: unique_ptr< DIR, std::function >
> Nit: remove the spaces separating the triangular brackets from other stuff 
could also use MakeScopedCleanup here


-- 
To view, visit http://gerrit.cloudera.org:8080/4502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idd058382e4519b323aebb4c992d9088496a341cc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [util] minor clean-up on kudu::Subprocess

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [util] minor clean-up on kudu::Subprocess
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4502/1/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:

PS1, Line 353: unique_ptr< DIR, std::function >
Nit: remove the spaces separating the triangular brackets from other stuff 
(i.e. unique_ptr> fd_dir).

Also, the compiler can't infer the second argument on its own the way it did 
when this used a shared_ptr?


PS1, Line 445:   if (state_ != kRunning) {
 : return Status::IllegalState("Sub-process is not running");
 :   }
I don't know about this. My impression is that the intent here was to be 
symmetric w.r.t. CheckAndOffer() and other similar functions; that is, to start 
every such function with an assertion that we're in the right state. Likely 
they could be downgraded to DCHECKs though.

Same for Start().


-- 
To view, visit http://gerrit.cloudera.org:8080/4502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idd058382e4519b323aebb4c992d9088496a341cc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [util] minor clean-up on kudu::Subprocess

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4502

Change subject: [util] minor clean-up on kudu::Subprocess
..

[util] minor clean-up on kudu::Subprocess

Do not call CHECK_EQ() in case where it's possible to report
on error via the return value.

Change-Id: Idd058382e4519b323aebb4c992d9088496a341cc
---
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/util/subprocess.cc
2 files changed, 20 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/4502/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Idd058382e4519b323aebb4c992d9088496a341cc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin