Michael Ho has posted comments on this change. Change subject: IMPALA-561: Allow multiple callbacks in a thread resource pool. ......................................................................
Patch Set 1: (13 comments) http://gerrit.cloudera.org:8080/#/c/2430/1/be/src/runtime/thread-resource-mgr-test.cc File be/src/runtime/thread-resource-mgr-test.cc: Line 94: MultiCallbacks > Since the RegisterPool/UnregisterPool paths are affected, can you exercise Done http://gerrit.cloudera.org:8080/#/c/2430/1/be/src/runtime/thread-resource-mgr.cc File be/src/runtime/thread-resource-mgr.cc: Line 79: UpdatePoolQuotas > While we do need to update per_pool_quota_, I think we can avoid having thi I am not so sure. In theory, if you have a high enough system_threads_quota_, num_available_threads() may still be > 0 after previous round of callbacks invocation. So, even if per_pool_quota_ may decrease after registering a new pool, it may still be large enough to allow callbacks to be invoked. That said, the behavior in UpdatePoolQuota() seems to favor older callbacks and I am not sure the reasoning behind it. Please note that the first check in ScheduleThreads() is done without lock so it shouldn't be too expensive. Line 109: callback > callbacks Done Line 110: callback > them Done Line 112: only scanner threads call this > "this is only called with any frequency on (a) the scanner thread completio Done Line 114: // TODO: reconsider this. > is this still really helpful? Removed. Line 136: for > I wonder if we can avoid this work when new_pool != NULL, i.e. a pool was a Please see replies above. http://gerrit.cloudera.org:8080/#/c/2430/1/be/src/runtime/thread-resource-mgr.h File be/src/runtime/thread-resource-mgr.h: Line 76: TODO: this is manageable now since it just needs to call into the io : /// mgr. What's the best model for something more general. > I know this isn't part of your change, but does this make any sense to you? Not sure if we need to generalize it for now until we have more use cases for it. May be this needs to be integrated with the scheduler. Comment removed. Line 86: ResourcePool > not really related to your change, but I wish we could avoid calling this t It's ThreadResourceMgr::ResourcePool so it's not too bad. Line 118: Returns an index : /// to be used when removing the callback. The index is unique for each callback : /// and is monotonically increasing until wrap around occurs. > I think it would be better to hide these details from the caller, which I d Comments moved to the function definition instead. Line 133: ScheduleThreads > does this need to be public? the behavior was basically private before, rig Good point. Made it a private function. Also, renamed the function to InvokeCallback(). Line 200: thread_callbacks_ > "...to the callback function registered, or NULL if the callback for this i Done Line 200: boost::unordered_map > Since the keys are just integers from [0, thread_callbacks_.size()], it mig Done -- To view, visit http://gerrit.cloudera.org:8080/2430 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iddfff1feef0b59d407994ad3bc560166acbfa623 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-HasComments: Yes
