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 5: (16 comments) http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/call.h File src/kudu/subprocess/call.h: http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/call.h@34 PS1, Line 34: > I'll let Andrew to comment/update on this as it should be more clear if com Done 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@128 PS1, Line 128: RETURN_NOT_OK(Thread::Create("subprocess", "writer", &SubprocessServer::SendMessagesThread, > > Could we move cb into the call instead of storing a pointer to it? I don't see a clear way to do this given the thread-safety considerations around concurrent calls. http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.cc@140 PS1, Line 140: auto cb = sync.AsStdStatusCallback(); : auto call = make_shared<SubprocessCall>(req, resp, &cb); : RETURN_NOT_OK(QueueCall(call)); > Hmm, I don't see there is required order for shutdown. But I'll let Andrew Not really; left a note. http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.cc@148 PS1, Line 148: // down the queues. We do the atomic exchange to avoid multiple threads > warning: loop variable is copied but only used as const reference; consider Done http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.cc@171 PS1, Line 171: std::lock_guard<simple_spinlock> l(in_flight_lock_); > It's possible the pipe was closed because the subprocess hit an exception, Done http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.cc@174 PS1, Line 174: for (const auto& id_and_call : calls) { > Yeah agreed that we needn't change the behavior here, but a TODO with desir Done http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.cc@186 PS1, Line 186: if (s.IsEndOfFile()) { > Consider BlockingDrainTo to reduce the number of wake ups under heavy load. Done http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.cc@191 PS1, 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 > The Java side can respond a message without ID if it cannot parse the proto Made this fatal instead of CHECKing. http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.cc@209 PS1, Line 209: // is shutting down. > This is a tight loop, which probably isn't what you wanted. Done http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.cc@215 PS1, Line 215: for (const auto& resp : resps) { : if (!resp.has_id()) { : LOG(FATAL) << Substitute("Received invalid response: $0", : pb_util::SecureDebugString(resp)); : } : } : vec > This is an approximation, right? There's no strict enforcement that the fir Done http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.cc@232 PS1, Line 232: for (auto& call_and_resp : calls_and_resps) { > Maybe use DrainTo to reduce the cost of waking up when under heavy load? Done http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.cc@253 PS1, Line 253: // haven't either. > Seems reasonable to add optional deadlines to BlockingPut and BlockingGet, That's true, though I'm going to punt on this; it'd definitely belong in its own patch, given we'd be changing the return type of BlockingPut() and BlockingGet() to reflect timeouts and shutdowns instead of simple succeed/failure. http://gerrit.cloudera.org:8080/#/c/15185/4/src/kudu/subprocess/server.cc File src/kudu/subprocess/server.cc: http://gerrit.cloudera.org:8080/#/c/15185/4/src/kudu/subprocess/server.cc@153 PS4, Line 153: } > warning: loop variable is copied but only used as const reference; consider Done http://gerrit.cloudera.org:8080/#/c/15185/4/src/kudu/subprocess/server.cc@181 PS4, Line 181: DCHECK(message_protocol_) << "message protocol is not initialized"; > warning: passing result of std::move() as a const reference argument; no mo Done http://gerrit.cloudera.org:8080/#/c/15185/4/src/kudu/subprocess/server.cc@257 PS4, Line 257: call_by_id_.erase(id_and_call); > warning: the parameter 'call' is copied for each invocation but only used a Done http://gerrit.cloudera.org:8080/#/c/15185/4/src/kudu/subprocess/subprocess_server-test.cc File src/kudu/subprocess/subprocess_server-test.cc: http://gerrit.cloudera.org:8080/#/c/15185/4/src/kudu/subprocess/subprocess_server-test.cc@69 PS4, Line 69: const string kHello = "hello world"; > warning: 'kHello' is a static definition in anonymous namespace; static is 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: 5 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 00:11:10 +0000 Gerrit-HasComments: Yes
