Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15185 )
Change subject: [cpp] KUDU-2971: protobuf-based wrapper for subprocesses ...................................................................... Patch Set 9: (15 comments) 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: If we're bu > Ah, interesting -- cmake is able to handle this in add_library. Could you Moved this into test-only bits. It's only used by unit tests. There might be some fancier stuff we can do to use add_dependencies(), but I haven't been able to figure it out in a way that also works with dist-test. http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/CMakeLists.txt@54 PS8, Line 54: ${ECHO_JAR} > Is this acceptable to make kudu_subprocess to depend on kudu_test_util? Do Good catch. I think this was here from a previous version of the patch that mixed test and production code. 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@95 PS8, Line 95: ondSuccess(Subpr > Can you comment on when can RespondSuccess() called after already running t Done Put these in-line with the early returns. http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/server.h@243 PS8, Line 243: > nit: remove? Done http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/server.h@260 PS8, Line 260: ResponseQueue inbound_response_queue_; > nit: name it to call_queue_ to match with response_queue_? Or the other way Done http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/server.h@265 PS8, Line 265: // start times which is useful for dead > Doc? Oops, this is unused. http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/server.h@274 PS8, Line 274: // name > Is 'mutable' really needed here? Done http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/server.cc File src/kudu/subprocess/server.cc: http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/server.cc@46 PS8, Line 46: > nit: space. Same below. Done http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/server.cc@64 PS8, Line 64: a timeou > nit: a timeout error. Done http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/server.cc@138 PS8, Line 138: next_id_++ > Do you think we should worry about we have many requests so that the id exc No. int64_max is 9,223,372,036,854,775,807. If we assume a constant stream of 1M requests per second (I've seen at most 75k rps), it'd take almost 300k years to exceed this. http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/server.cc@218 PS8, Line 218: pb > nit: alignment. Done 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@85 PS8, Line 85: eads = 0) { > nit: kudu-subprocess-echo.jar Done http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/subprocess_server-test.cc@91 PS8, Line 91: tring bin_dir = > nit: remove? Done http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/subprocess_server-test.cc@103 PS8, Line 103: p > I think you mean to use 'p' here for specifying message parser threads in Done http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/subprocess_server-test.cc@148 PS8, Line 148: threads.emplace_back([&, t] { : for (int i = 0; i < kNumPerThread; i++) { : > If ASSERT_OK() failed above, this would not work. Consider using ScopedCle Done -- 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: 9 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 21:59:51 +0000 Gerrit-HasComments: Yes
