Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15185 )
Change subject: [cpp] KUDU-2971: protobuf-based wrapper for subprocesses ...................................................................... Patch Set 8: (7 comments) some nits http://gerrit.cloudera.org:8080/#/c/15185/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15185/3//COMMIT_MSG@9 PS3, Line 9: patchs patch http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/CMakeLists.txt File src/kudu/subprocess/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/CMakeLists.txt@50 PS8, Line 50: ${ECHO_JAR} Ah, interesting -- cmake is able to handle this in add_library. Could you add a comment clarifying that this element is here just for dependency (or something else)? BTW, maybe add_dependencies() is better in this case? http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/CMakeLists.txt@54 PS8, Line 54: kudu_test_util Is this acceptable to make kudu_subprocess to depend on kudu_test_util? Does it make kudu-master and kudu-tserver binaries will depend on symbols pulled from kudu_test_util? http://gerrit.cloudera.org:8080/#/c/15185/3/src/kudu/subprocess/call.h File src/kudu/subprocess/call.h: http://gerrit.cloudera.org:8080/#/c/15185/3/src/kudu/subprocess/call.h@99 PS3, Line 99: nit: drop? http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/server.h File src/kudu/subprocess/server.h: http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/server.h@265 PS8, Line 265: std::atomic<int64_t> overflow_timeout_; Doc? http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/server.h@274 PS8, Line 274: mutable Is 'mutable' really needed here? http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/subprocess_server-test.cc File src/kudu/subprocess/subprocess_server-test.cc: http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/subprocess_server-test.cc@148 PS8, Line 148: for (auto& t : threads) { : t.join(); : } If ASSERT_OK() failed above, this would not work. Consider using ScopedCleanup instead to join the threads. -- To view, visit http://gerrit.cloudera.org:8080/15185 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id611e1c683df2721fd058f753b8686a688a5990d Gerrit-Change-Number: 15185 Gerrit-PatchSet: 8 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 20 Feb 2020 19:46:41 +0000 Gerrit-HasComments: Yes
