Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9527 )
Change subject: IMPALA-6609: Fix ownership of class members in KrpcDataStreamRecvr ...................................................................... Patch Set 1: (3 comments) Yeah, we really need to rethink this in milestone 2. http://gerrit.cloudera.org:8080/#/c/9527/1/be/src/runtime/krpc-data-stream-recvr.h File be/src/runtime/krpc-data-stream-recvr.h: http://gerrit.cloudera.org:8080/#/c/9527/1/be/src/runtime/krpc-data-stream-recvr.h@201 PS1, Line 201: own most of the : /// counters below if the only exception is the one you marked "Not owned", how about saying "own the counters below unless noted otherwise"? Otherwise we're left guessing which are which. http://gerrit.cloudera.org:8080/#/c/9527/1/be/src/runtime/krpc-data-stream-recvr.cc File be/src/runtime/krpc-data-stream-recvr.cc: http://gerrit.cloudera.org:8080/#/c/9527/1/be/src/runtime/krpc-data-stream-recvr.cc@451 PS1, Line 451: Only update the 'deferred_rpc_tracker_' if the queue is still open. I think a "why" rather than "what" would be more useful here. Something like: // If Is_cancelled_ was set, must avoid accessing deferred_rpc_tracker_ since the parent tracker can be destroyed after cancellation. Or whatever the actual reason is. But actually, this seems weird since in Close() you say this stuff must live until Close(), and why can we be calling TakeOverEarlySender() (or any of these) after Close() is called? I guess you are saying we do allow these calls after Close() due to the service threads not knowing it's been closed, and we're really using the is_cancalled_ flag as a conservative check for whether Close() might have been called? http://gerrit.cloudera.org:8080/#/c/9527/1/be/src/runtime/krpc-data-stream-recvr.cc@568 PS1, Line 568: Note that the parent profile doesn't hold any ownership to these : // profiles. I think this will be hard to decifer, is there a more direct thing we can say. Is this correct? These profiles are not owned by the parent profile since they must outlive the exchange node which owns the parent profile. -- To view, visit http://gerrit.cloudera.org:8080/9527 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings 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 <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-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 07 Mar 2018 17:23:57 +0000 Gerrit-HasComments: Yes
