Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/24905 )
Change subject: KUDU-3806: add deadline-bounded WaitAndCollect() ...................................................................... Patch Set 3: (9 comments) Thank you for the comments Alexey! 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: ASSERT_FALSE(timed_out); : ASSERT_EQ("out\n", out); : ASSERT_EQ("err\n", err); : ASSERT_TRUE(WIFEXITED(wait_status)); : ASSERT_EQ(0, WEXITSTATUS(wait_status)); > Here and below: what's the reason behind this mix of EXPECT_xxx and ASSERT_ Changed to use ASSERTs only. 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: Yepp makes sense, added 2 new tests. 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 sce Added TestWaitAndCollectExpiredDeadline let me know what you think. 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: MonoDelt > In all the call sites of WaitAndCollect, it's necessary to first find curre Makes sense changed to MonoDelta. 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 clas Done http://gerrit.cloudera.org:8080/#/c/24905/1/src/kudu/util/subprocess.cc@299 PS1, Line 299: > Prefer allocating local helper objects on the stack, not on the heap. If t Done http://gerrit.cloudera.org:8080/#/c/24905/1/src/kudu/util/subprocess.cc@653 PS1, Line 653: exit_status, info_str)); > Should there be a check to enforce the invariant mentioned in the descripti Done http://gerrit.cloudera.org:8080/#/c/24905/1/src/kudu/util/subprocess.cc@683 PS1, Line 683: , fds.size()); : size_t i = 0; > readability nit: prefer using strings::Substitute() here and elsewhere for Done http://gerrit.cloudera.org:8080/#/c/24905/1/src/kudu/util/subprocess.cc@903 PS1, Line 903: > nit: for brevity, it's possible to use '{}' instead of explicitly calling t Done -- 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: 3 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: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Wed, 23 Sep 2026 11:04:31 +0000 Gerrit-HasComments: Yes
