Michael Ho 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) 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 reactor thread could have sent the reply and deleted InboundCall object already. Seems safer to store the transfer size upfront before replying. http://gerrit.cloudera.org:8080/#/c/8914/9/be/src/rpc/impala-service-pool.cc@164 PS9, Line 164: c->GetTransferSize() Same here. http://gerrit.cloudera.org:8080/#/c/8914/9/be/src/rpc/impala-service-pool.cc@169 PS9, Line 169: c->GetTransferSize() Same here. 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 think we generally try to add this to all functions where it makes sense, I believe we have NODISCARD in the Status class declaration so we no longer have to do it manually for each function which returns Status. 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->GetTransferSize()) here too given what we did in AddEarlyClosedSender() ? 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); ? -- 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 <l...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 25 Jan 2018 00:54:47 +0000 Gerrit-HasComments: Yes
