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 7: (13 comments) Looking good. Some minor comments. http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/rpc/impala-service-pool.cc File be/src/rpc/impala-service-pool.cc: http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/rpc/impala-service-pool.cc@161 PS7, Line 161: if (queue_status == kudu::rpc::QueueStatus::QUEUE_SHUTDOWN) { We may reach here when trying to enqueue after the shutdown of the queue has begun. In which case, shouldn't we release the memory to MemTracker as the incoming RPC hasn't yet been enqueued ? http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/rpc/impala-service-pool.cc@164 PS7, Line 164: } else { Should we release the memory in MemTracker here too ? 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 Not needed ? http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/runtime/exec-env.cc@343 PS7, Line 343: // Bump thread cache to 1GB to reduce contention for TCMalloc central : // list's spinlock. : if (FLAGS_tcmalloc_max_total_thread_cache_bytes == 0) { : FLAGS_tcmalloc_max_total_thread_cache_bytes = 1 << 30; : } These lines should be gone after rebase. http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/runtime/krpc-data-stream-mgr.h File be/src/runtime/krpc-data-stream-mgr.h: http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/runtime/krpc-data-stream-mgr.h@293 PS7, Line 293: // nit: /// http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/runtime/krpc-data-stream-mgr.h@294 PS7, Line 294: // specific receiver. Not owned. http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/runtime/krpc-data-stream-mgr.h@295 PS7, Line 295: MemTracker* mem_tracker_; = nullptr; http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/runtime/krpc-data-stream-mgr.h@297 PS7, Line 297: MemTracker which is used by the DataStreamService to track memory for incoming : // requests. It is this class's responsibility to transfer memory from this tracker It's not very clear from the comments what the relationship between mem_tracker_ and incoming_request_tracker_ is. If I understand it correctly, mem_tracker_ here mostly accounts for memory for holding the requests of early senders before they are transferred to the receiver. The memory is transferred from 'incoming_request_tracker_' which tracks the memory allocated for the payload of the RPC dequeued from the service queue of DataStreamService. http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/runtime/krpc-data-stream-mgr.h@298 PS7, Line 298: // requests. It is this class's responsibility to transfer memory from this tracker. Not owned. http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/runtime/krpc-data-stream-mgr.h@299 PS7, Line 299: MemTracker* incoming_request_tracker_; = nullptr; http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/runtime/krpc-data-stream-mgr.h@481 PS7, Line 481: /// 'mem_tracker_'. Please add a comment on when this is used. Please also state the side effect (e.g. the incoming sidecars are not supposed to be accessed anymore). http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/runtime/krpc-data-stream-mgr.h@485 PS7, Line 485: /// 'incoming_request_tracker_'. Please add a comment on when this is used. http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/runtime/krpc-data-stream-recvr.cc File be/src/runtime/krpc-data-stream-recvr.cc: http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/runtime/krpc-data-stream-recvr.cc@130 PS7, Line 130: /// MemTracker attached to 'recvr_'. Please add that it's no longer safe to access the inbound sidecars after this. The request protobuf buffer is still okay to access. -- 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: 7 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: Mon, 22 Jan 2018 20:32:19 +0000 Gerrit-HasComments: Yes
