Matthew Jacobs 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 that as well? E.g. if there was a second registered pool, upon its unregistration the first pool's callbacks could be called. 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 this fn call ScheduleThreads() since no new tokens would have become available. What do you think? Line 109: callback callbacks Line 110: callback them Line 112: only scanner threads call this "this is only called with any frequency on (a) the scanner thread completion path and (b) pool registration/unregistration"? Line 114: // TODO: reconsider this. is this still really helpful? Line 136: for I wonder if we can avoid this work when new_pool != NULL, i.e. a pool was added. See my comment on l79. 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? Line 86: ResourcePool not really related to your change, but I wish we could avoid calling this the overly generic "ResourcePool"... If it's not too annoying can you consider changing this to something like ThreadResourcePool? (Feel free to push back :) ) 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 don't think it really needs to know. Can we just call this a unique id for this cb fn? I think the only important thing is that the caller knows to provide the same unique id to Remove. Line 133: ScheduleThreads does this need to be public? the behavior was basically private before, right? Should this be called OfferThreads? Line 200: boost::unordered_map Since the keys are just integers from [0, thread_callbacks_.size()], it might be a little simpler for this to just be a vector. Line 200: thread_callbacks_ "...to the callback function registered, or NULL if the callback for this id was unregistered." Otherwise it's not clear why you need num_callbacks_ below. -- 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-HasComments: Yes
