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: (25 comments) http://gerrit.cloudera.org:8080/#/c/15185/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15185/9//COMMIT_MSG@9 PS9, Line 9: patchs > patch Done http://gerrit.cloudera.org:8080/#/c/15185/9//COMMIT_MSG@20 PS9, Line 20: a > drop Done 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? Done 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 le Ack 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 goi Thanks for the help here Done 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. Done 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 s Done http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/server.h@126 PS9, Line 126: being > "is" Done http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/server.h@127 PS9, Line 127: mutable > nit: drop? Done http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/server.h@130 PS9, Line 130: int64_t > ah, please scratch this :) Ack http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/server.h@133 PS9, Line 133: start_time_ > Maybe, store the deadline? Given that timeout can be changed via SetFlag() Good points, though I'm hesitant to allow for such dynamic timeouts because the deadlines can then become quite out of order with respect to ID if we change our timeouts at runtime. When we check deadlines, we use low IDs to approximate older calls, and thus, earlier deadlines. I added a comment, though if you feel very strongly about this I can change it. http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/server.h@270 PS9, Line 270: int64_t > nit: maybe, introduce a typedef and use it here and elsewhere when referrin Done 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@20 PS9, Line 20: #include <signal.h> > nit: maybe use <csignal> ? Done http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/server.cc@98 PS9, Line 98: : next_id_(1), closing_(false), > nit: why not to put every item in its own line? Done http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/server.cc@99 PS9, Line 99: new Subprocess(std::move(subprocess_argv)) > nit: use std::make_shared<Subprocess>(std::move(subprocess_argv)) instead? Done 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' altog CMPXCHG takes a ref as its first arg, so the compiler complains. Added a comment. http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/server.cc@191 PS9, Line 191: // TODO(awong): getting an error here indicates that this server and the : // underlying subprocess are not in sync (e.g. not speaking the same : // protocol). We should consider either crashing here, or restarting the : // subprocess. > Indeed, maybe at least add DCHECK() then? Done http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/server.cc@199 PS9, Line 199: VLOG(2) << "failed to put response onto inbound queue"; > Here and elsewhere where going out of thread loop tasks: consider adding LO Sure. I didn't want this to be too verbose since I originally ran with 20 responder threads. 3 is more palatable though. http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/server.cc@210 PS9, Line 210: ok() > nit: maybe, check for the particular error (like IsAborted()) here and add Done http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/server.cc@221 PS9, Line 221: calls_and_resps > nit: maybe, add calls_and_resps.reserve(resps.size()); ? Done 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 Done 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." Done 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_ Done 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. TEST_F(SubprocessServerTest, RestartSubprocess) { constexpr int kNumRestarts = 100; double total_time_secs = 0; for (int i = 0; i < kNumRestarts; i++) { Stopwatch sw(Stopwatch::ALL_THREADS); sw.start(); ASSERT_OK(ResetSubprocessServer()); sw.stop(); total_time_secs += sw.elapsed().wall_seconds(); } LOG(INFO) << Substitute("total: $0, average: $1", total_time_secs, total_time_secs / static_cast<double>(kNumRestarts)); } subprocess_server-test.cc:123] total: 5.377794849999999, average: 0.05377794 Not too bad, seems like. 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 st Ah, this was useful before but now it is not. 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: Fri, 21 Feb 2020 08:14:38 +0000 Gerrit-HasComments: Yes
