Matthew Jacobs has posted comments on this change. Change subject: IMPALA-561: Allow multiple callbacks in a thread resource pool. ......................................................................
Patch Set 2: (12 comments) http://gerrit.cloudera.org:8080/#/c/2430/2/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: Line 985: pool->ReleaseThreadToken(false); ? http://gerrit.cloudera.org:8080/#/c/2430/2/be/src/exec/hdfs-scan-node.h File be/src/exec/hdfs-scan-node.h: Line 498: thread_callback_id_ -1 indicates no registered callback. http://gerrit.cloudera.org:8080/#/c/2430/2/be/src/runtime/thread-resource-mgr.cc File be/src/runtime/thread-resource-mgr.cc: Line 117: count num_callbacks_invoked? Line 119: size size is pretty generic sounding, it might be easier to read if removing this and just use thread_callbacks_.size() (or possibly a better name) http://gerrit.cloudera.org:8080/#/c/2430/2/be/src/runtime/thread-resource-mgr.h File be/src/runtime/thread-resource-mgr.h: Line 116: index in hdfs-scan-node now we're calling it an 'id', can we do the same here? Line 122: from all callbacks not necessary Line 122: idx id? (and the parameter name?) Line 122: Note that the : /// entry in thread_callbacks_ is not removed. Instead, the entry is set to NULL so : /// the callback index won't be reused until wrap around occurs. This part isn't really relevant to the consumer of this contract. Can you hide this implementation detail in the .cc file? It's also hinted by the comment on thread_callbacks_. Line 175: . : /// Scheduling is done nix Scheduling as it's no longer used. Can combine these sentences, e.g.: ... is exhausted, in round-robin order. Line 195: could will Line 199: registered callbacks Maybe add: ..., i.e. the number of non-NULL entries in thread_callbacks_. Line 202: next The next index into thread_callbacks_ to invoke, in round-robin order. -- 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: 2 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
