Adar Dembo has posted comments on this change. Change subject: threadpool: token-based task sequencing ......................................................................
Patch Set 6: (10 comments) http://gerrit.cloudera.org:8080/#/c/6874/6/src/kudu/util/debug-util.h File src/kudu/util/debug-util.h: PS6, Line 105: { > add 'const' specifier? Done http://gerrit.cloudera.org:8080/#/c/6874/6/src/kudu/util/threadpool.cc File src/kudu/util/threadpool.cc: PS6, Line 333: DEALLOCATED > AVAILABLE Done PS6, Line 344: DEALLOCATED > AVAILABLE Done Line 435: SequenceSlot* slot = slot_index ? slots_[*slot_index].get() : nullptr; > paranoid nit: does it make sense to add something like Done Line 473: DCHECK(state == SequenceSlot::ASSIGNED || > Why not (state != SequenceSlot::AVAILABLE)? I find explicitly enumerating the allowed states to be more clear. It's also easier to notice mistakes if/when a new state is introduced. Yes, the guarantee is that if DoSubmit() was called with a slot index, it must have originated with a token, which means the slot cannot be AVAILABLE. I'm not a fan of EMPTY because I can see myself confusing it with "the slot's queue is empty of tasks" (i.e. ASSIGNED). PS6, Line 708: } > Does it make sense to add CHECK(!task) for the case new_state == ASSIGNED? Yes, I'll add that. http://gerrit.cloudera.org:8080/#/c/6874/6/src/kudu/util/threadpool.h File src/kudu/util/threadpool.h: PS6, Line 177: ; > add 'const' specifier? Done Line 304: boost::optional<SequenceToken::SlotIndex> slot_index); > Could you add a comment explaining the expected usage with slot_index (slot Sure. Line 398: void CheckStateTransition(State new_state, const boost::optional<Task>& task); > add 'const' specifier? Done PS6, Line 453: Queues > nit: Queue No, this is a container of slots, and each slot is logically a 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 <a...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
