Tim Armstrong 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/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 seems important to reject incoming RPCs. 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 lock? Might help in some cases where system is already overloaded. 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 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 runtime type so static_cast should work. 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. http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/runtime/krpc-data-stream-mgr.cc File be/src/runtime/krpc-data-stream-mgr.cc: http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/runtime/krpc-data-stream-mgr.cc@176 PS2, Line 176: service_mem_tracker_->Release(transfer_size); > The process limit is checked by looking at TC-malloc and the buffer pool, r I think generally we should be conservative and call Consume() before Release(). I can see Michael's argument in this case we could cause unnecessary failures in that way. I think in either case the probability of problems is low so this seems ok. 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. It should also work for this purpose. I think having three similar public APIs is really confusing. -- 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 06:31:55 +0000 Gerrit-HasComments: Yes
