Todd Lipcon 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 SERIAL vs CONCURRENT was a property of a task rather than a token, and it confused me a bit. Maybe rephrase a little so it says something like: A ThreadPoolToken operates in one of two ExecutionModes: - SERIAL: tasks submitted via this token run one at a time - CONCURRENT: tasks may be run concurrently blah blah ... so it's just a little more obvious that the modes are set per token and not per task 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 sharing one big threadpool across multiple usage contexts? Is it to take advantage of per-token metrics? Might be worth noting something about this in the doc. Line 190: // Allocates a new token for use in token-based task submission. can you document the requirements around token lifetime relative to pool lifetime? namely I think you have to destroy all tokens before destroying the pool? 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 PS11, Line 298: whom nit: which Line 330: ~ThreadPoolToken(); I think worth noting whether the token-owner is responsible for doing a Shutdown() and/or Wait() before allowing it to be destructed. 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 in use than actually defined, so if we aren't using all three, maybe we can use this new API as an opportunity to consolidate? PS11, Line 342: If a task is in flight nit: should be plural, since you can make a CONCURRENT token, right? Line 361: private: What's the thread safety on the members here? Are they protected by the threadpool's lock? think it's worth a block comment explaining. -- 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
