Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9282 )
Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/9282/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9282/3//COMMIT_MSG@20 PS3, Line 20: 20 > 5% Done http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/rpc/impala-service-pool.cc File be/src/rpc/impala-service-pool.cc: http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/rpc/impala-service-pool.cc@155 PS2, Line 155: CheckLimitExceeded > Why don't we call AnyLimitExceeded()? If we're over the process limit it se I thought about that option but I am concerned about calling Gc in the middle of the Rpc path. Updated the patch to make AnyLimitExceeded() to not GC if specified by caller. http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/impala-service-pool.cc File be/src/rpc/impala-service-pool.cc: http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/impala-service-pool.cc@159 PS3, Line 159: c->DiscardTransfer(); > Maybe drop the lock at this point to avoid calling free() while holding the Done http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/rpc-mgr-test-base.h File be/src/rpc/rpc-mgr-test-base.h: http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/rpc-mgr-test-base.h@135 PS3, Line 135: TakeoverService > nit: take over is two words, so TakeOverService Done http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/rpc-mgr-test-base.h@234 PS3, Line 234: dynamic_cast<PingServiceImpl*>(ping_impl)->mem_tracker())); > dynamic_cast<> here and below shouldn't be necessary - don't need to check Done http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/rpc-mgr-test.cc File be/src/rpc/rpc-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/rpc-mgr-test.cc@186 PS3, Line 186: dynamic_cast > static_cast should also work here and below. Done http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/runtime/mem-tracker.h File be/src/runtime/mem-tracker.h: http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/runtime/mem-tracker.h@281 PS3, Line 281: bool CheckLimitExceeded() const { return limit_ >= 0 && limit_ < consumption(); } > Why not use the existing public LimitExceeded() or AnyLimitExceeded() APIs. I am a bit concerned by the fact that exceeding the memory limit may lead to GcMemory being called in the middle of some critical path. May be an alternative would be to make LimitExceeded() take an argument to skip Gc and simply return true. Implemented this idea in the new patch. -- To view, visit http://gerrit.cloudera.org:8080/9282 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9 Gerrit-Change-Number: 9282 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@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: Fri, 16 Feb 2018 08:46:30 +0000 Gerrit-HasComments: Yes
