Repository: kudu Updated Branches: refs/heads/master 69e657808 -> 643c92212
[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 <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/643c9221 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/643c9221 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/643c9221 Branch: refs/heads/master Commit: 643c922128f09854e08e0970707aa344215fa898 Parents: 69e6578 Author: Alexey Serbin <[email protected]> Authored: Wed Sep 21 18:18:13 2016 -0700 Committer: Adar Dembo <[email protected]> Committed: Mon Sep 26 19:29:22 2016 +0000 ---------------------------------------------------------------------- src/kudu/benchmarks/tpch/tpch_real_world.cc | 12 ++++-- .../full_stack-insert-scan-test.cc | 10 +++-- src/kudu/util/subprocess.cc | 39 +++++++++++++------- 3 files changed, 42 insertions(+), 19 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/643c9221/src/kudu/benchmarks/tpch/tpch_real_world.cc ---------------------------------------------------------------------- diff --git a/src/kudu/benchmarks/tpch/tpch_real_world.cc b/src/kudu/benchmarks/tpch/tpch_real_world.cc index 3a622a2..c9f03c6 100644 --- a/src/kudu/benchmarks/tpch/tpch_real_world.cc +++ b/src/kudu/benchmarks/tpch/tpch_real_world.cc @@ -42,6 +42,7 @@ #include <stdlib.h> #include <sys/types.h> #include <sys/stat.h> +#include <csignal> #include <boost/bind.hpp> #include <glog/logging.h> @@ -300,9 +301,14 @@ void TpchRealWorld::MonitorDbgenThread(int i) { SleepFor(MonoDelta::FromMilliseconds(100)); } } - dbgen_proc->Kill(9); - int ret; - dbgen_proc->Wait(&ret); + Status s = dbgen_proc->Kill(SIGKILL); + if (!s.ok()) { + LOG(FATAL) << "Failed to send SIGKILL to dbgen: " << s.ToString(); + } + s = dbgen_proc->Wait(nullptr); + if (!s.ok()) { + LOG(FATAL) << "Failed to await for dbgen exit: " << s.ToString(); + } } void TpchRealWorld::RunQueriesThread() { http://git-wip-us.apache.org/repos/asf/kudu/blob/643c9221/src/kudu/integration-tests/full_stack-insert-scan-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/integration-tests/full_stack-insert-scan-test.cc b/src/kudu/integration-tests/full_stack-insert-scan-test.cc index 2ea11c8..dc2e3dc 100644 --- a/src/kudu/integration-tests/full_stack-insert-scan-test.cc +++ b/src/kudu/integration-tests/full_stack-insert-scan-test.cc @@ -305,11 +305,15 @@ void FullStackInsertScanTest::DoTestScans() { LOG(INFO) << "Doing test scans on table of " << kNumRows << " rows."; gscoped_ptr<Subprocess> stat = MakePerfStat(); + if (stat) { + ASSERT_OK(stat->Start()); + } gscoped_ptr<Subprocess> record = MakePerfRecord(); - if (stat) stat->Start(); - if (record) record->Start(); + if (record) { + ASSERT_OK(record->Start()); + } - NO_FATALS(ScanProjection(vector<string>(), "empty projection, 0 col")); + NO_FATALS(ScanProjection({}, "empty projection, 0 col")); NO_FATALS(ScanProjection({ "key" }, "key scan, 1 col")); NO_FATALS(ScanProjection(AllColumnNames(), "full schema scan, 10 col")); NO_FATALS(ScanProjection(StringColumnNames(), "String projection, 1 col")); http://git-wip-us.apache.org/repos/asf/kudu/blob/643c9221/src/kudu/util/subprocess.cc ---------------------------------------------------------------------- diff --git a/src/kudu/util/subprocess.cc b/src/kudu/util/subprocess.cc index b270dcf..fb296d4 100644 --- a/src/kudu/util/subprocess.cc +++ b/src/kudu/util/subprocess.cc @@ -27,6 +27,7 @@ #include <sys/wait.h> #include <unistd.h> +#include <functional> #include <memory> #include <string> #include <vector> @@ -45,9 +46,8 @@ #include "kudu/util/errno.h" #include "kudu/util/status.h" -using std::unique_ptr; -using std::shared_ptr; using std::string; +using std::unique_ptr; using std::vector; using strings::Split; using strings::Substitute; @@ -314,12 +314,14 @@ static int pipe2(int pipefd[2], int flags) { #endif Status Subprocess::Start() { - CHECK_EQ(state_, kNotStarted); - EnsureSigPipeDisabled(); - - if (argv_.size() < 1) { + if (state_ != kNotStarted) { + return Status::IllegalState( + Substitute("$0: illegal sub-process state", state_)); + } + if (argv_.empty()) { return Status::InvalidArgument("argv must have at least one elem"); } + EnsureSigPipeDisabled(); vector<char*> argv_ptrs; for (const string& arg : argv_) { @@ -348,8 +350,8 @@ Status Subprocess::Start() { DIR* fd_dir = nullptr; RETURN_NOT_OK_PREPEND(OpenProcFdDir(&fd_dir), "Unable to open fd dir"); - shared_ptr<DIR> fd_dir_closer(fd_dir, CloseProcFdDir); - + unique_ptr<DIR, std::function<void(DIR*)>> fd_dir_closer(fd_dir, + CloseProcFdDir); int ret = fork(); if (ret == -1) { return Status::RuntimeError("Unable to fork", ErrnoToString(errno), errno); @@ -417,12 +419,18 @@ Status Subprocess::Start() { Status Subprocess::DoWait(int* ret, int options) { if (state_ == kExited) { - *ret = cached_rc_; + if (ret != nullptr) { + *ret = cached_rc_; + } return Status::OK(); } - CHECK_EQ(state_, kRunning); + if (state_ != kRunning) { + return Status::IllegalState( + Substitute("$0: illegal sub-process state", state_)); + } - int rc = waitpid(child_pid_, ret, options); + int proc_exit_info; + int rc = waitpid(child_pid_, &proc_exit_info, options); if (rc == -1) { return Status::RuntimeError("Unable to wait on child", ErrnoToString(errno), @@ -434,13 +442,18 @@ Status Subprocess::DoWait(int* ret, int options) { CHECK_EQ(rc, child_pid_); child_pid_ = -1; - cached_rc_ = *ret; + cached_rc_ = proc_exit_info; state_ = kExited; + if (ret != nullptr) { + *ret = proc_exit_info; + } return Status::OK(); } Status Subprocess::Kill(int signal) { - CHECK_EQ(state_, kRunning); + if (state_ != kRunning) { + return Status::IllegalState("Sub-process is not running"); + } if (kill(child_pid_, signal) != 0) { return Status::RuntimeError("Unable to kill", ErrnoToString(errno),
