Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9446 )
Change subject: IMPALA-6554: Fix a race in DequeueDeferredRpc() ...................................................................... Patch Set 3: (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 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::RespondAndReleaseRpc() ? We have all the necessary params in 'TransmitDataCtx'. Or we could still keep this and call KrpcDataStreamMgr<TransmitDataCtx>::RespondAndReleaseRpc() instead of duplicating the code. 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 this code here. Another option is to pass a 'bool should_release_mem_tracker' or something. But that's a bit shabby too. I'm fine with either if we can't think of something better. Just wanted to consider another option. -- 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: 3 Gerrit-Owner: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Comment-Date: Thu, 01 Mar 2018 00:04:21 +0000 Gerrit-HasComments: Yes
