Adar Dembo 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: (13 comments) http://gerrit.cloudera.org:8080/#/c/15185/9/build-support/run_dist_test.py File build-support/run_dist_test.py: http://gerrit.cloudera.org:8080/#/c/15185/9/build-support/run_dist_test.py@156 PS9, Line 156: # Restore the symlinks to the chrony binaries; tests expect to find them in : # same directory as the test binaries themselves. Update the comment? http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/CMakeLists.txt File src/kudu/subprocess/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/CMakeLists.txt@41 PS9, Line 41: COMMAND ./gradlew :kudu-subprocess-echo:jar I'm still not sold on this (vs. add_jar(), despite the duplication), but let's see what it feels like to interact with the build first. http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/CMakeLists.txt@54 PS9, Line 54: ${ECHO_JAR} You want it in the list of sources? Isn't that kind of weird? We're not going to link the JAR into the library... Why not add_dependency(kudu_subprocess ${ECHO_JAR}) instead? http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/CMakeLists.txt@75 PS9, Line 75: endif() ADD_KUDU_TEST already considers NO_TESTS, so you can move this to L73. http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/server.h File src/kudu/subprocess/server.h: http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/server.h@122 PS9, Line 122: sending of the request doesn't intersect with : // the calling of the callback Nit: "intersect" is an unusual choice; perhaps "sending of the request is serialized with respect to the invocation of the callback"? Or "...is mutually exclusive with respect to...". Basically something that draws on more typical concurrency terminology. http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/server.h@126 PS9, Line 126: being "is" http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.cc File src/kudu/subprocess/server.cc: http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.cc@253 PS1, Line 253: // haven't either. > That's true, though I'm going to punt on this; it'd definitely belong in it Works for me. http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/server.cc File src/kudu/subprocess/server.cc: http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/server.cc@150 PS9, Line 150: bool expected = false; Can't you just do CMPXCHG(false, true) and avoid declaring 'expected' altogether? http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/server.cc@257 PS9, Line 257: call_by_id_.erase(id_and_call); You can do a range-based erase since you're iterating in order over the map. Something like: for (map entries from oldest to newest): if entry deadline has passed: timed_out_calls.emplace_back(*cur) else: map.erase(map.begin, cur) break process timed_out_calls http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/subprocess_server-test.cc File src/kudu/subprocess/subprocess_server-test.cc: http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/subprocess_server-test.cc@83 PS9, Line 83: // Reset the subprocess server with to account for any new configurations. "Resets the subprocess server to account for any new configuration." http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/subprocess_server-test.cc@88 PS9, Line 88: Env* env = Env::Default(); KuduTest provides env_ http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/subprocess_server-test.cc@107 PS9, Line 107: return server_->Init(); What's the latency for this? Just curious. http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/subprocess_server-test.cc@239 PS9, Line 239: unique_ptr<thread> t; Why does it need to be in a unique_ptr? Can't it just be declared on the stack on L241? -- 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: Fri, 21 Feb 2020 06:06:15 +0000 Gerrit-HasComments: Yes
