Sailesh Mukil has posted comments on this change. ( )

Change subject: IMPALA-6609: Fix ownership of class members in 

Patch Set 1:

File be/src/runtime/krpc-data-stream-recvr.h:
PS1, Line 163:   /// Row schema.
Add "Not owned."
PS1, Line 182: client_
Not your change, but I think we should we rename this to buffer_pool_client_, 
or something similar.
'client_' is very very generic.
PS1, Line 201: own most of the
             :   /// counters below
> if the only exception is the one you marked "Not owned", how about saying "
Add "These profiles are guaranteed to live as long as their parent 'profile_' 
under the receiver's current shared ownership model."
File be/src/runtime/
PS1, Line 636:
Maybe I'm missing something, but I'm not able to understand why this was 
PS1, Line 411: recvr_->deferred_rpc_tracker()
We access the deferred_rpc_tracker() here too. Should we check for cancellation 
in this function too?
PS1, Line 644: deferred_rpc_tracker_->Close();
This seems a little brittle, although it would work for us. In the header, you 
mention that this must be accessed through a sender queue's lock, however, 
we're not doing that here.

It still is semantically correct since we close the queues before accessing 
this, but future changes may miss this subtlety and bring the issue back.

I think a brief comment is warranted here explaining this.

To view, visit
To unsubscribe, visit

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3378496e2201b16c662b9daa634333480705c61a
Gerrit-Change-Number: 9527
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Comment-Date: Wed, 07 Mar 2018 19:30:07 +0000
Gerrit-HasComments: Yes

Reply via email to