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: (13 comments) Thanks for the comments. Please see my replies inline and PS9, which also contains a test for clearing out deferred RPCs. This was a TODO left from Tim's review. 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 ha Done http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/rpc/impala-service-pool.cc@164 PS7, Line 164: mem_tracker_->Release(c->GetTransferSize()); > Should we release the memory in MemTracker here too ? 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 > Not needed ? I think we generally try to add this to all functions where it makes sense, and I couldn't think of a case where we want to ignore a failure here. Can you give an example? 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. Thanks for the heads up, I'll double check. 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: /// Done http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/runtime/krpc-data-stream-mgr.h@294 PS7, Line 294: /// specific receiver. Not owned. > Not owned. Done http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/runtime/krpc-data-stream-mgr.h@295 PS7, Line 295: MemTracker* mem_tracker_ = nullptr; > = nullptr; Done http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/runtime/krpc-data-stream-mgr.h@298 PS7, Line 298: /// requests. Memory for new incoming requests is initially tracked against this tracker > Not owned. Done 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. Memory for new incoming requests is initially tracked against this tr > It's not very clear from the comments what the relationship between mem_tra Yes, I tried to make the comment more clear. http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/runtime/krpc-data-stream-mgr.h@299 PS7, Line 299: /// before the requests are handed over to the service. It is the services' (here: this > = nullptr; Done http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/runtime/krpc-data-stream-mgr.h@481 PS7, Line 481: void Maintenance(); > Please add a comment on when this is used. Please also state the side effec Done http://gerrit.cloudera.org:8080/#/c/8914/7/be/src/runtime/krpc-data-stream-mgr.h@485 PS7, Line 485: /// tracked against 'mem_tracker_'. After calling this function, the inbound sidecars of > Please add a comment on when this is used. Done 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_'. After calling this function, the inbound sidecars > Please add that it's no longer safe to access the inbound sidecars after th 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: Wed, 24 Jan 2018 23:09:13 +0000 Gerrit-HasComments: Yes
