Andrew Wong has posted comments on this change. Change subject: threadpool: token-based task sequencing ......................................................................
Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/6874/6/src/kudu/util/threadpool.cc File src/kudu/util/threadpool.cc: Line 473: DCHECK(state == SequenceSlot::ASSIGNED || Why not (state != SequenceSlot::AVAILABLE)? Just checking that I understand this correctly, this implies that the caller knows the SequenceSlot at slot_index is not in the AVAILABLE state, right? And this is ensured by the fact that only a SequenceToken will DoSubmit with a slot_index. I know it's gone through name changes already, but how do you feel about EMPTY instead of AVAILABLE? My thinking, at least for this bit of logic, is that it's clearer that a task shouldn't be assigned to an empty slot http://gerrit.cloudera.org:8080/#/c/6874/6/src/kudu/util/threadpool.h File src/kudu/util/threadpool.h: Line 304: boost::optional<SequenceToken::SlotIndex> slot_index); Could you add a comment explaining the expected usage with slot_index (slot_index must refer to a SequenceSlot that is occupied by a token)? PS6, Line 453: Queues nit: Queue -- 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: 6 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
