Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24905 )
Change subject: KUDU-3806: add deadline-bounded WaitAndCollect() ...................................................................... Patch Set 2: Code-Review+1 (9 comments) http://gerrit.cloudera.org:8080/#/c/24905/1/src/kudu/util/subprocess-test.cc File src/kudu/util/subprocess-test.cc: http://gerrit.cloudera.org:8080/#/c/24905/1/src/kudu/util/subprocess-test.cc@173 PS1, Line 173: EXPECT_FALSE(timed_out); : EXPECT_EQ("out\n", out); : EXPECT_EQ("err\n", err); : ASSERT_TRUE(WIFEXITED(wait_status)); : EXPECT_EQ(0, WEXITSTATUS(wait_status)); Here and below: what's the reason behind this mix of EXPECT_xxx and ASSERT_xxx? http://gerrit.cloudera.org:8080/#/c/24905/1/src/kudu/util/subprocess-test.cc@241 PS1, Line 241: } One more test scenario that I'd think of adding is this: * a sub-process runs and outputs some data into its stdout/stderr * the sub-process times out * WaitAndCollect() returns Status::OK() as expected, detects the timeout, and returns the data that's been output to the sub-process's stdout/stderr as expected For the sub-process's output, there might be two options: * a static output that's easy to verify against the reference * dynamic output such as `while true; do echo "x"; sleep 1; done` or similar: the point here isn't verifying against the expected reference, but rather make sure nothing funny happens with the readers when timeout happens, making sure WaitAndCollect returns Status::OK() even if there was some pending data to be read when the timeout struck. What do you think? http://gerrit.cloudera.org:8080/#/c/24905/1/src/kudu/util/subprocess-test.cc@242 PS1, Line 242: In the context of covering edge cases, does it make sense to add a test scenario that starts a sub-process that's running for some time (e.g., similar to TestWaitAndCollectTimeout that runs for 1000 seconds) and then calls WaitAndCollect on it with a deadline that's already in the past at the time of starting the subprocess? IIUC, it should behave simlar to TestWaitAndCollectTimeout, but it might make sense to have explicit coverage for such a corner case. http://gerrit.cloudera.org:8080/#/c/24905/1/src/kudu/util/subprocess.h File src/kudu/util/subprocess.h: http://gerrit.cloudera.org:8080/#/c/24905/1/src/kudu/util/subprocess.h@148 PS1, Line 148: MonoTime In all the call sites of WaitAndCollect, it's necessary to first find current time, then add necessary MonoDelta, and then pass it as the 'deadline' argument for this function. And the delta is re-computed from MonoTime into MonoDelta before calling corresponding libev function. Do we really want the absolute time notation for deadline or we might rather prefer the delta notation (i.e. using MonoDelta) here? http://gerrit.cloudera.org:8080/#/c/24905/1/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: http://gerrit.cloudera.org:8080/#/c/24905/1/src/kudu/util/subprocess.cc@268 PS1, Line 268: }; Does it make sense to prohibit copying and assigning instances of this class? Can copies of ev::timer with the same dynamic loop introduce some sort of UB? http://gerrit.cloudera.org:8080/#/c/24905/1/src/kudu/util/subprocess.cc@299 PS1, Line 299: new DeadlineHelper Prefer allocating local helper objects on the stack, not on the heap. If the optional semantics is required, use the std::optional<T> wrapper. One of the reasons is contention in tcmalloc under heavy load and concurrency; in this case it's not crucial, though. http://gerrit.cloudera.org:8080/#/c/24905/1/src/kudu/util/subprocess.cc@653 PS1, Line 653: // Collect only the streams the caller asked for; each must be piped (i.e. not Should there be a check to enforce the invariant mentioned in the description: The deadline is enforced while draining the pipes, so at least one of 'stdout_out'/'stderr_out' must be non-null for it to bound a child that neither exits nor writes. http://gerrit.cloudera.org:8080/#/c/24905/1/src/kudu/util/subprocess.cc@683 PS1, Line 683: "failed to kill timed-out child " << program_ << ": " : << s.ToString(); readability nit: prefer using strings::Substitute() here and elsewhere for formatting the log message http://gerrit.cloudera.org:8080/#/c/24905/1/src/kudu/util/subprocess.cc@903 PS1, Line 903: MonoTime() nit: for brevity, it's possible to use '{}' instead of explicitly calling the named type's constructor, 'MonoTime()' -- To view, visit http://gerrit.cloudera.org:8080/24905 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I582ea89b0ada4f723695df8d98d7e5bc0bbc7f7f Gerrit-Change-Number: 24905 Gerrit-PatchSet: 2 Gerrit-Owner: Marton Greber <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Tue, 22 Sep 2026 19:58:40 +0000 Gerrit-HasComments: Yes
