Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9446 )
Change subject: IMPALA-6554: Fix a race in DequeueDeferredRpc() ...................................................................... Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/9446/1/be/src/runtime/krpc-data-stream-recvr.cc File be/src/runtime/krpc-data-stream-recvr.cc: http://gerrit.cloudera.org:8080/#/c/9446/1/be/src/runtime/krpc-data-stream-recvr.cc@337 PS1, Line 337: RespondAndReleaseRpc so in order for this to be correct w.r.t. Close(), the lock must be held while calling it, right? Let's update the function comment. Also, this function seems badly named. AFAICT, recvr_->mem_tracker() is only used to track deferred RPCs, and so this should only be called when responding to a deferred RPC, correct? If so, how about we call it RespondAndReleaseDeferredRpc or something? Also, should mem_tracker_ be labeled to indicate it is only for deferred RPCs? Once the RPC is deseralized, it's tracked by the BP client's mem-tracker directly, not the recvr_->mem_tracker_ right? http://gerrit.cloudera.org:8080/#/c/9446/1/be/src/runtime/krpc-data-stream-recvr.cc@438 PS1, Line 438: 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(); given that the other callsites are holding the lock, why is it not okay to do so here. RespondSuccess() is where the payload is freed, right? So not holding the lock means that Close() returns while the recvr resources are not yet released, right? That seems like it might cause RM trouble for us when we actually bring this under reservations. -- 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: 1 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Comment-Date: Mon, 26 Feb 2018 18:17:56 +0000 Gerrit-HasComments: Yes
