Michael Ho 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); > ? We were missing a call to release thread token here (see TryAcquireThreadToken() above). Please let me know if it's not necessary somehow. Also updated commit message about it. 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. Done 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? Done. Renamed to num_invoked. Line 119: size > size is pretty generic sounding, it might be easier to read if removing thi Done 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? Done Line 122: idx > id? (and the parameter name?) Done Line 122: from all callbacks > not necessary Comment moved to .cc file instead. 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 h Comment moved to .cc file instead. Line 175: . : /// Scheduling is done > nix Scheduling as it's no longer used. Can combine these sentences, e.g.: Comments rephrased. Line 195: could > will Done Line 199: registered callbacks > Maybe add: ..., i.e. the number of non-NULL entries in thread_callbacks_. Done Line 202: next > The next index into thread_callbacks_ to invoke, in round-robin order. 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: 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
