Todd Lipcon has posted comments on this change. Change subject: threadpool: token-based task sequencing ......................................................................
Patch Set 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/6874/8/src/kudu/util/threadpool.cc File src/kudu/util/threadpool.cc: Line 278: if (!s->not_running_cond.TimedWait(delta)) { on spurious wakeup, won't this wait longer than expected? I think implementing WaitUntil in terms of a deadline cond var wait would be better, and convert this one to use the deadline? http://gerrit.cloudera.org:8080/#/c/6874/8/src/kudu/util/threadpool.h File src/kudu/util/threadpool.h: PS8, Line 188: Upon completion, 't' has been released not following this bit. I'm reading this to mean "Release()" below, but that doesn't make sense. I'd expect 'this' to be released prior to the move. -- 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: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
