[Impala-ASF-CR] IMPALA-6554: Fix a race in DequeueDeferredRpc()

["Sailesh Mukil (Code Review)" <ge...@cloudera.org>]

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 <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Mar 2018 00:04:21 +0000
Gerrit-HasComments: Yes