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 4: (9 comments) Thanks for the reviews and discussion. Please see PS5 and my inline comments. 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 constructio Done 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 Following the discussion in krpc-data-stream-mgr.cc I left this unchanged for now. 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 w I updated the change to pass the service memtracker to the data stream manager. This allows us to Release the memory there before we either track it again in the data stream manager or call AddBatch on a receiver. However, we face a similar pattern there, that we release memory before making a function call. It does not cross the KRPC service boundary so it may be less prone to errors, but still seems not ideal. Do you have a pattern to recommend for these cases? Should we always try to pass the sender's tracker along with the payload so that the receiver of a call can release memory before consuming it from its own tracker? Should we introduce a small helper class that represents a memory allocation, keeps a pointer to its current_tracker_ and has a Move(MemTracker* new_tracker) method to release memory from current_tracker_, consume it from new_tracker, and set current_tracker_ = new_tracker. Such a class could also DCHECK in its d'tor if its memory has not been released. http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/rpc/rpc-mgr.h@165 PS3, Line 165: /// Passed to new services. : MemTracker* mem_tracker_; > Is this still needed ? Removed. 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: MemTracker* stream_mgr_tracker = obj_pool_->Add( > Is there a more descriptive name we could use for this one. E.g. "Data Stre Done, thank you for coming up with a suggestion. http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/runtime/exec-env.cc@343 PS3, Line 343: // Bump thread cache to 1GB to reduce contention for TCMalloc central > nit: extra blank line Done 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: ctx->serialized_size() > Wouldn't the majority of the changes be not needed if we add a new interfac Done 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 Ack http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/runtime/krpc-data-stream-recvr.cc@373 PS3, Line 373: Consume > Here too Ack -- 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: 4 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: Wed, 17 Jan 2018 20:41:45 +0000 Gerrit-HasComments: Yes
