Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/8914 )
Change subject: IMPALA-6193: Track memory of incoming data streams ...................................................................... Patch Set 6: (8 comments) Thanks for the review, please see PS6. http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/rpc/impala-service-pool.cc File be/src/rpc/impala-service-pool.cc: http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/rpc/impala-service-pool.cc@58 PS5, Line 58: DCHECK(mem_tracker_ != nullptr); > nit: extra indentation. Done http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/runtime/krpc-data-stream-mgr.cc File be/src/runtime/krpc-data-stream-mgr.cc: http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/runtime/krpc-data-stream-mgr.cc@172 PS5, Line 172: const EndDataStreamRequestPB* request, EndDataStreamResponsePB* response, > I generally think it's better to Consume() before Release()ing to avoid und Done, thanks for the suggestion. Let me know if you'd like me to factor the code into an own method, since it's repeated in AddEarlySender() above. http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/runtime/krpc-data-stream-mgr.cc@212 PS5, Line 212: // time without considering the remaining number of senders. As a consequence, > Has the memory been released at this point? It hasn't but was about to in RespondSuccess(). I made it explicit by calling DiscardTransfer(). http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/runtime/krpc-data-stream-mgr.cc@268 PS5, Line 268: } > Has the rpc_context memory been released at this point? RespondSuccess() would release it, I made it more precise by calling DiscardTransfer(). However, EndDataStream RPCs don't have sidecars, so only the serialized PB messages will be discarded, making up less than 100 bytes. http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/runtime/krpc-data-stream-mgr.cc@387 PS5, Line 387: // to senders which sent EOS RPC so all query fragments will eventually be cancelled. > We haven't actually released the memory at this point, have we? Should we b Done http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/runtime/krpc-data-stream-mgr.cc@391 PS5, Line 391: for (const unique_ptr<TransmitDataCtx>& ctx : senders_queue.waiting_sender_ctxs) { > Same question here. Done http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/runtime/krpc-data-stream-recvr.cc File be/src/runtime/krpc-data-stream-recvr.cc: http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/runtime/krpc-data-stream-recvr.cc@410 PS5, Line 410: } > Is the memory released? No, at this point it is still held. I moved the release to the right place. http://gerrit.cloudera.org:8080/#/c/8914/5/be/src/runtime/krpc-data-stream-recvr.cc@458 PS5, Line 458: { > Same here At this point the rpc_context was even destroyed - this was a bug. I still haven't found a way to trigger deferred RPCs in a test which is why I hadn't caught this. -- To view, visit http://gerrit.cloudera.org:8080/8914 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2df1204d2483313a8a18e5e3be6cec9e402614c4 Gerrit-Change-Number: 8914 Gerrit-PatchSet: 6 Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 18 Jan 2018 03:45:16 +0000 Gerrit-HasComments: Yes
