David Ribeiro Alves has posted comments on this change. Change subject: threadpool: token-based task sequencing ......................................................................
Patch Set 8: (11 comments) http://gerrit.cloudera.org:8080/#/c/6874/8/src/kudu/util/debug-util.h File src/kudu/util/debug-util.h: PS8, Line 105: bool HasCollected() const { docs http://gerrit.cloudera.org:8080/#/c/6874/8/src/kudu/util/threadpool-test.cc File src/kudu/util/threadpool-test.cc: PS8, Line 418: t3 = std::move(t1); agree with tidy bot that these uses after move are weird http://gerrit.cloudera.org:8080/#/c/6874/8/src/kudu/util/threadpool.cc File src/kudu/util/threadpool.cc: PS8, Line 173: (std::shared_ptr<Runnable> nit: make_shared? PS8, Line 188: pool_->CheckNotPoolThreadUnlocked(); what is the point of this check? can you leave a comment? PS8, Line 258: s->state() == ThreadPool::SequenceSlot::State::RUNNABLE || : s->state() == ThreadPool::SequenceSlot::State::RUNNING || : s->state() == ThreadPool::SequenceSlot::State::QUIESCING isn't this equivalent to IsActive() PS8, Line 275: s->state() == ThreadPool::SequenceSlot::State::RUNNABLE || : s->state() == ThreadPool::SequenceSlot::State::RUNNING || : s->state() == ThreadPool::SequenceSlot::State::QUIESCING same. http://gerrit.cloudera.org:8080/#/c/6874/8/src/kudu/util/threadpool.h File src/kudu/util/threadpool.h: PS8, Line 140: Wait variants shutdown is also thread safe, right? PS8, Line 156: Waits until all the tasks submitted via this token are completed and also : // marks the token as unusable for future submissions. mind reverting this sentence? I reads like we wait first and then close the close the token PS8, Line 212: Tasks submitted via one token are fairly prioritized with respect to tasks : // submitted via another token. not sure I understand this... how are tasks on a token prioritized over another token? how do you chose which token get priority? PS8, Line 360: QUIESCED -> AVAILABLE: why do we need to transition the state back to available? PS8, Line 424: StackTrace running_submit_stack; what's the perf impact of this? I realize this is a debug only thing but we don't want debug modes to be abnormally slow. Consider gating this behind a flag if the perf impact is considerable. -- 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
