Michael Ho has posted comments on this change. Change subject: IMPALA-561: Allow multiple callbacks in a thread resource pool. ......................................................................
Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/2430/4/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: Line 699: thread_callback_id_ > how about 'thread_avail_cb_id_' to be more specific? Done http://gerrit.cloudera.org:8080/#/c/2430/4/be/src/runtime/thread-resource-mgr.cc File be/src/runtime/thread-resource-mgr.cc: Line 52: thread_callbacks_.clear(); > would it make sense to DCHECK that all callbacks were removed? Good suggestion. Actually, it's more appropriate to have it at pool unregistration instead. Line 95: // until wrap around occurs. > this comment doesn't seem to match the code given that size() can't wrap. Oh right ! Comment updated. Line 108: thread_callbacks_[id] = NULL; > does this leave the vector sparse under certain patterns? or is it always A Yes, this can be a sparse vector but the majority of time there is only 1 entry only so not worth optimizing for. The sparse vector makes implementing round robin a bit simpler as we don't have don't have to adjust the next_callback_idx_ when callbacks are unregistered. Line 121: num_callbacks_ > why do we use this instead of just doing thread_callbacks_.size()? Due to the sparse vector, num_callbacks_ <= thread_callbacks_.size(). -- 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: 4 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-HasComments: Yes
