Todd Lipcon has posted comments on this change. Change subject: WIP: threadpool: token-based sequencing ......................................................................
Patch Set 1: (1 comment) this seems like a reasonable approach. Potential concerns for discussion: - does this affect the performance of a normal ThreadPool that doesn't use the token feature? From looking at the code it seems like it would just be a few branches and no extra allocation, etc, but would be good to be sure of that - not sure about using a string for a token vs having a more opaque token, such as ThreadPool::ObtainSequencer() returning a 'Sequencer' struct (which could well be just an int or somesuch) - docs wise, would be good to document fairness and starvation-freedom properties. Particularly, the fact that this exhibits neither :-D Probably fine since I think we would typically use this feature in threadpools with no max thread count specified. (is it worth actually prohibiting use of tokens in a pool with a max thread count? or is it just "YMMV" situation?) http://gerrit.cloudera.org:8080/#/c/6874/1/src/kudu/util/threadpool-test.cc File src/kudu/util/threadpool-test.cc: Line 392: result += c; I think you'd probably want to add a random sleep at the beginning of this lambda, so that if they did accidentally execute in parallel, it would be more likely to actually misorder -- To view, visit http://gerrit.cloudera.org:8080/6874 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If46dc34212027b6ea5dbc2ead7c7af8be57f2c70 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
