Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8914 )
Change subject: IMPALA-6193: Track memory of incoming data streams ...................................................................... Patch Set 3: (7 comments) Did a pass, focusing on integration with MemTracker infra. I think the significant issue is whether we can use TryConsume() instead of Consume() to avoid going over the mem limit. Using Consume() is better than before, but would be nice to use our best practices if feasible. http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/rpc/impala-service-pool.h File be/src/rpc/impala-service-pool.h: http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/rpc/impala-service-pool.h@64 PS3, Line 64: Maybe make it "MemTracker* const" since it's not modified after construction. http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/rpc/impala-service-pool.cc File be/src/rpc/impala-service-pool.cc: http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/rpc/impala-service-pool.cc@141 PS3, Line 141: mem_tracker_->Consume(c->GetTransferSize()); Is it reasonable to call TryConsume() here and handle the failure, e.g. in the same way as a full queue below? The Consume() + check for mem limit exceeded asynchronously pattern is deprecated since it lets us go over the mem limit and it's easy to accidentally omit the asynchronous checks. http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/rpc/impala-service-pool.cc@180 PS3, Line 180: mem_tracker_->Release(incoming->GetTransferSize()); Leaving the memory temporarily untracked here isn't great. Is there a way we can hand it off to the service so the service tracks it. Otherwise it seems safer to call Release() after Handle() and double-count it temporarily. http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/runtime/exec-env.cc@328 PS3, Line 328: new MemTracker(-1, "Data Stream Manager", mem_tracker_.get())); Is there a more descriptive name we could use for this one. E.g. "Data Stream Queued RPC Calls" or something like that? http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/runtime/krpc-data-stream-mgr.cc File be/src/runtime/krpc-data-stream-mgr.cc: http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/runtime/krpc-data-stream-mgr.cc@175 PS3, Line 175: Consume Similar question as earlier about TryConsume() - can we write this in a way such that it will synchronously handle OOM? http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/runtime/krpc-data-stream-recvr.cc File be/src/runtime/krpc-data-stream-recvr.cc: http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/runtime/krpc-data-stream-recvr.cc@324 PS3, Line 324: Consume Same question about TryConsume http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/runtime/krpc-data-stream-recvr.cc@373 PS3, Line 373: Consume Here too -- 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: 3 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: Tue, 09 Jan 2018 22:40:19 +0000 Gerrit-HasComments: Yes
