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 3: (3 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: class SubprocessCall { > Yeah, fair point about clarity. This is around as safe as it needs to be: Agreed that adding the call to the map _before_ sending the request is less complex. Just doc'ing what's needed out of thread safety should help me (or others) what is necessary and what isn't. 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. > To be clear, it's safe to update 'cb_' after calling it, but it's unsafe to Makes sense; I assumed "the request" referred to the entirety of SubprocessCall. 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@174 PS1, Line 174: WARN_NOT_OK(s, "failed to receive response from the subprocess"); > Hao and I discussed this briefly. I think there was some consensus that we Yeah agreed that we needn't change the behavior here, but a TODO with desired (or possible) semantics would be good. -- 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: 3 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: Tue, 18 Feb 2020 17:59:15 +0000 Gerrit-HasComments: Yes
