Hao Hao 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: (11 comments) Overall looks good to me, thanks a lot for working on the patch! http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/CMakeLists.txt File src/kudu/subprocess/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/CMakeLists.txt@38 PS1, Line 38: set(ECHO_JAR ${EXECUTABLE_OUTPUT_PATH}/kudu-subprocess-echo.jar) > Hmm, but we use add_jar() for kudu-hive, and the list of dependencies is qu Yeah, I think we probably would want to switch to gradle for src/kudu/hms/CMakeLists.txt as well. 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: RespondSuccess() Can you comment on when can RespondSuccess() called after already running the callback? http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/server.h@243 PS8, Line 243: to nit: remove? http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/server.h@260 PS8, Line 260: SubprocessCallQueue outbound_call_queue_; nit: name it to call_queue_ to match with response_queue_? Or the other way around? 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. http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/server.cc@64 PS8, Line 64: an error nit: a timeout error. 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 exceeds the maximum of int64_t? http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/server.cc@218 PS8, Line 218: pb_ nit: alignment. 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: kudu-subprocess.jar nit: kudu-subprocess-echo.jar http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/subprocess_server-test.cc@91 PS8, Line 91: subprocess_home nit: remove? http://gerrit.cloudera.org:8080/#/c/15185/8/src/kudu/subprocess/subprocess_server-test.cc@103 PS8, Line 103: w I think you mean to use 'p' here for specifying message parser threads in java side, https://github.com/apache/kudu/blob/master/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java#L90 -- 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: 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 06:22:08 +0000 Gerrit-HasComments: Yes
