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 9: (13 comments) few nits 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 http://gerrit.cloudera.org:8080/#/c/15185/9//COMMIT_MSG@20 PS9, Line 20: a drop 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@127 PS9, Line 127: mutable nit: drop? http://gerrit.cloudera.org:8080/#/c/15185/9/src/kudu/subprocess/server.h@130 PS9, Line 130: int64_t It's unlikely it's going to overflow, but using unsigned type would help in that case to preserve consistent ordering in SubprocessServer::call_by_id_ I'm just curious: is there a requirement to use a signed type here? 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() in run-time, storing the deadline is more consistent then. Also, since checking deadline happens often (like every 50 ms), getting rid of extra '+' operation for MonoTime/MonoDelta saves a few CPU cycles. 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 referring to call identifier? 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> ? 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? 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? 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? 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 LOG(INFO) since in release mode if something happens and it's not actually closing, it would be easier to troubleshoot in case if subprocess server stuck due to thread exiting like this. 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 a DCHECK() for other cases? 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()); ? -- 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:39:47 +0000 Gerrit-HasComments: Yes
