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 9: (6 comments) Thanks for the review comments. I'm not particularly happy with the fact that I had missed some of the issues. If you have a suggestion how we can increase the test coverage to catch those, please let me know. http://gerrit.cloudera.org:8080/#/c/8914/9/be/src/rpc/impala-service-pool.cc File be/src/rpc/impala-service-pool.cc: http://gerrit.cloudera.org:8080/#/c/8914/9/be/src/rpc/impala-service-pool.cc@109 PS9, Line 109: c->GetTransferSize() > Not sure it's safe to assume that c is still valid at this point. The react Done http://gerrit.cloudera.org:8080/#/c/8914/9/be/src/rpc/impala-service-pool.cc@164 PS9, Line 164: c->GetTransferSize() > Same here. Done http://gerrit.cloudera.org:8080/#/c/8914/9/be/src/rpc/impala-service-pool.cc@169 PS9, Line 169: c->GetTransferSize() > Same here. Done http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/rpc/rpc-mgr.h@126 PS7, Line 126: WARN_UNUSED_RESULT > I believe we have NODISCARD in the Status class declaration so we no longer I didn't know we had that. The commit message of the change that introduces it mentions that it has no effect on GCC 4.9.2 (https://gerrit.cloudera.org/c/7253/), so I think we still need WARN_UNUSED_RESULT. http://gerrit.cloudera.org:8080/#/c/8914/9/be/src/runtime/krpc-data-stream-mgr.cc File be/src/runtime/krpc-data-stream-mgr.cc: http://gerrit.cloudera.org:8080/#/c/8914/9/be/src/runtime/krpc-data-stream-mgr.cc@122 PS9, Line 122: for (const unique_ptr<EndDataStreamCtx>& ctx : early_senders.closed_sender_ctxs) { > Shouldn't we also call mem_tracker_->Release(ctx->rpc_context->GetTransferS Yes, added it. Do you know of a test that exercised this code? http://gerrit.cloudera.org:8080/#/c/8914/9/be/src/runtime/krpc-data-stream-recvr.cc File be/src/runtime/krpc-data-stream-recvr.cc: http://gerrit.cloudera.org:8080/#/c/8914/9/be/src/runtime/krpc-data-stream-recvr.cc@407 PS9, Line 407: if (UNLIKELY(!status.ok())) { > Missing DiscardAndReleaseRpcMemory(ctx->rpc_context); ? Done -- 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: 9 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, 25 Jan 2018 03:26:16 +0000 Gerrit-HasComments: Yes
