Adar Dembo has posted comments on this change. Change subject: threadpool: token-based task sequencing ......................................................................
Patch Set 11: (9 comments) http://gerrit.cloudera.org:8080/#/c/6874/11/src/kudu/util/threadpool.h File src/kudu/util/threadpool.h: PS11, Line 135: // Furthermore, tasks submitted to tokens constructed via ExecutionMode::SERIAL : // are guaranteed not to run in parallel. > the first time I read through this (before reading the code) I thought the Will update. Line 138: // Tasks submitted without a token or via ExecutionMode::CONCURRENT tokens are > can you remind me what the advantage of CONCURRENT tokens is, vs just shari Suppose you have n contexts sharing a single threadpool. If tokens aren't used for submission, there's no way for one of the contexts to atomically: 1. Abort any of its queued tasks. 2. If any are running, wait for them to finish. 3. Prohibit future submission. ThreadPool::Shutdown() is inappropriate because it applies to the entire pool, not just to this one context. I don't know exactly what to call this property, so the description is a little vague ("...can be used to block on logical groups of tasks"). Per-context metrics is useful, but it's not necessary for correctness the way this is. Line 190: // Allocates a new token for use in token-based task submission. > can you document the requirements around token lifetime relative to pool li Done PS11, Line 190: // Allocates a new token for use in token-based task submission. : // : // There is no limit on the number of tokens that may be allocated. > nit: I think this comment belongs below the enum rather than above it Er, why? It is a "new paragraph" within the NewToken() documentation, so I think it should be part of the "Allocates a new token..." block of text. PS11, Line 298: whom > nit: which Done Line 330: ~ThreadPoolToken(); > I think worth noting whether the token-owner is responsible for doing a Shu OK. The destructor will call Shutdown(), so I'll note that doing so explicitly is not a requirement. PS11, Line 332: // Submits a function using the kudu Closure system. : Status SubmitClosure(Closure c) WARN_UNUSED_RESULT; : : // Submits a function bound using boost::bind(&FuncName, args...). : Status SubmitFunc(boost::function<void()> f) WARN_UNUSED_RESULT; : : // Submits a Runnable class. : Status Submit(std::shared_ptr<Runnable> r) WARN_UNUSED_RESULT; > do we still need all of these variants? I seem to recall that we have fewer All three variants are in use in unit tests. The functor variant is also used by peers, and the closure variant is also used by the message queue and raft consensus. PS11, Line 342: If a task is in flight > nit: should be plural, since you can make a CONCURRENT token, right? Done Line 361: private: > What's the thread safety on the members here? Are they protected by the thr Done -- 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: 11 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
