Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9446 )
Change subject: IMPALA-6554: Fix a race in DequeueDeferredRpc() ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/9446/2/be/src/runtime/krpc-data-stream-recvr.h File be/src/runtime/krpc-data-stream-recvr.h: http://gerrit.cloudera.org:8080/#/c/9446/2/be/src/runtime/krpc-data-stream-recvr.h@176 PS2, Line 176: exchange node > this receiver Done http://gerrit.cloudera.org:8080/#/c/9446/3/be/src/runtime/krpc-data-stream-recvr.cc File be/src/runtime/krpc-data-stream-recvr.cc: http://gerrit.cloudera.org:8080/#/c/9446/3/be/src/runtime/krpc-data-stream-recvr.cc@341 PS3, Line 341: RespondAndReleaseDeferredRpc > Why have this? Why can't we just use KrpcDataStreamMgr::RespondAndReleaseRp Good point. I refactored the code and moved it to DataStreamService instead. http://gerrit.cloudera.org:8080/#/c/9446/3/be/src/runtime/krpc-data-stream-recvr.cc@441 PS3, Line 441: // Release to MemTracker while still holding the lock to prevent race with Close(). : recvr_->mem_tracker()->Release(ctx->rpc_context->GetTransferSize()); : } : : // Responds to the sender to ack the insertion of the row batches. : status.ToProto(ctx->response->mutable_status()); : ctx->rpc_context->RespondSuccess(); > We have a dedicated function to do this, so it's not ideal to duplicate thi I prefer not to enqueue the reply under the lock if possible. Please see replies to previous comments for more details. Passing a bool seems more complicated IMHO. It's just two lines so don't think it's too bad to duplicate them here. -- To view, visit http://gerrit.cloudera.org:8080/9446 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4a3b0789633c7b8bc898381d509e2af769f0e069 Gerrit-Change-Number: 9446 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Comment-Date: Thu, 01 Mar 2018 06:12:40 +0000 Gerrit-HasComments: Yes
