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 4: (18 comments) Addressed partial comments and push what I have for now so that Andrew can pick it back. 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(SUBPROCESS_HOME ${EXECUTABLE_OUTPUT_PATH}/subprocess-home) > I don't think you want to use execute_process() here, as it means this code I chose to not use add_jar() as we are kind of duplicating the effort of adding/figuring out dependencies here (which we already did via Gradle). And if there is any dependencies need to be updated, we need to update it in two places. 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: class SubprocessCall { > Agreed that adding the call to the map _before_ sending the request is less I'll let Andrew to comment/update on this as it should be more clear if coming from him. http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/call.h@47 PS1, Line 47: MonoTime start_ > It's a simple int64_t so just return a copy. Done http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/call.h@71 PS1, Line 71: SubprocessResponsePB r > GSG forbids rvalue references outside of obvious usage (i.e. move construct Done http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/call.h@97 PS1, Line 97: This is important because the callback may : // destroy the request > Makes sense; I assumed "the request" referred to the entirety of Subprocess Ack http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/call.h@99 PS1, Line 99: // 'resp_' afterwards). Note that it is safe to update 'cb_' after the > Given that this is held while invoking the callback and that the callback m Done http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.h File src/kudu/subprocess/server.h: http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.h@42 PS1, Line 42: // Used by BlockingQueue to determine the size of messages. > Shouldn't be necessary given you're including call.h. Then again, why is ca Done http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.h@85 PS1, Line 85: > warning: single-argument constructors must be marked explicit to avoid unin 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@44 PS1, Line 44: : DEFINE_int32(subprocess_request_queue_size_bytes, 4 * 1024 * 1024, : "Maximum size in bytes of the outbound request queue"); : TAG_FLAG(subprocess_request_queue_size_bytes, advanced); : : DEFINE_int32(subprocess_response_queue_size_bytes, 4 * 1024 * 1024, : "Maximum size in bytes of the inbound response queue"); : TAG_FLAG(subprocess_response_queue_size_bytes, advanced); > What happens if a single request or response exceeds this amount? Does the Based on the implementation of BlockingQueue, it should allow the element to be added to the queue. Although I don't think we should have cases where the max message size exceeds the max queue size. http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.cc@64 PS1, Line 64: enqueue the > Nit: duplicative. Done http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.cc@112 PS1, Line 112: } : RETURN_NOT_OK(Thread::Create("subprocess", "reader", &SubprocessServer::ReceiveMessagesThread, : this, &read_thread_)); : RETURN_NOT_OK(Thread::Create("subprocess", "writer", &SubprocessServer::SendMessagesThread, : this, &write_thread_)); : RETURN_NOT_OK(Thread::Create("subprocess", "deadline-checker", : &SubprocessServer::CheckDea > Nit: the "Task" suffix suggests a one-time operation rather than an active Done http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.cc@128 PS1, Line 128: auto cb = sync.AsStdStatusCallback(); > Could we move cb into the call instead of storing a pointer to it? It appears to me cb (as a pointer) is used as indicator of whether the callback has been called or not. But I will leave it for Andrew to comment. http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.cc@134 PS1, Line 134: void SubprocessServer::Shutdown() { : if (closing_) { : return; : } : // Stop further work from happening by killing the subprocess and shutting : // down the queues. W > Should use a CAS to avoid multiple threads racing on Shutdown and executing Done http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.cc@140 PS1, Line 140: // racing on Shutdown. : bool expected = false; : if (!closing_.compare_exchange_s > Curious as to whether there's a specific shutdown order we must follow, and Hmm, I don't see there is required order for shutdown. But I'll let Andrew to comment. http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.cc@165 PS1, Line 165: call->RespondError(Status::ServiceUnavailable("subprocess is shutting down")); > Want to mirror this in SendMessagesTask? Done http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.cc@176 PS1, Line 176: // The underlying pipe was closed. We're likely shutting down. > Yeah, we could probably assert that we are closing_ since we'll only ever g Done http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.cc@188 PS1, Line 188: } > Assert on closing_? Done http://gerrit.cloudera.org:8080/#/c/15185/1/src/kudu/subprocess/server.cc@191 PS1, Line 191: while (!closing_.load()) { : SubprocessResponsePB resp; : if (!respon > Hmm, shouldn't we CHECK/DCHECK on this? Seems like it'd indicate a programm The Java side can respond a message without ID if it cannot parse the protobuf message. Although I cannot think of a valid case that Java side can receive an invalid/unexpected protobuf message. So we probably should assert here? -- 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: 4 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 19 Feb 2020 02:10:07 +0000 Gerrit-HasComments: Yes
