You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2016/11/30 17:49:37 UTC
[kudu-CR] threadpool: Allow zero-size task queue
Hello Dinesh Bhat,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5275
to review the following change.
Change subject: threadpool: Allow zero-size task queue
......................................................................
threadpool: Allow zero-size task queue
A zero-size task queue implies that the number of tasks running or
queued at a given time will never exceed max_threads. In reality, a the
queue is not actually bounded to 0, because a queue is always used by
the worker threads to claim tasks, however that is an implementation
detail.
Added a new test for this functionality and updated an existing
queue-related test that is now deterministic instead of racy.
Change-Id: I5abf40473ee813c625e0a02232d714aab2e65109
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
2 files changed, 33 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/5275/1
--
To view, visit http://gerrit.cloudera.org:8080/5275
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5abf40473ee813c625e0a02232d714aab2e65109
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
[kudu-CR] threadpool: Allow zero-size task queue
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: threadpool: Allow zero-size task queue
......................................................................
Patch Set 4:
Looped 1000x in RELEASE mode: http://dist-test.cloudera.org/job?job_id=mpercy.1480549133.28790
--
To view, visit http://gerrit.cloudera.org:8080/5275
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5abf40473ee813c625e0a02232d714aab2e65109
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] threadpool: Allow zero-size task queue
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: threadpool: Allow zero-size task queue
......................................................................
Patch Set 1:
> curious what the motivation was for this change?
The idea is that we can have a thread pool for Tablet Copy that has a queue size of 0 so that we create backpressure on the remote (ServiceUnavailable) when the queue becomes full.
--
To view, visit http://gerrit.cloudera.org:8080/5275
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5abf40473ee813c625e0a02232d714aab2e65109
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] threadpool: Allow zero-size task queue
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: threadpool: Allow zero-size task queue
......................................................................
Patch Set 1:
curious what the motivation was for this change?
--
To view, visit http://gerrit.cloudera.org:8080/5275
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5abf40473ee813c625e0a02232d714aab2e65109
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] threadpool: Allow zero-size task queue
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: threadpool: Allow zero-size task queue
......................................................................
Patch Set 3: -Code-Review
It turns out that ThreadPoolTest.TestRace is flaky on master. So I'm not super concerned about this:
http://dist-test.cloudera.org/job?job_id=mpercy.1480545421.2919
Although it would be good to know why it's crashing. Unfortunately, the logs are not being captured.
--
To view, visit http://gerrit.cloudera.org:8080/5275
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5abf40473ee813c625e0a02232d714aab2e65109
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] threadpool: Allow zero-size task queue
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: threadpool: Allow zero-size task queue
......................................................................
Patch Set 3:
cool, thanks for looping. this is one of those tricky areas where a small bug can have a huge impact!
--
To view, visit http://gerrit.cloudera.org:8080/5275
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5abf40473ee813c625e0a02232d714aab2e65109
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] threadpool: Allow zero-size task queue
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: threadpool: Allow zero-size task queue
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5275
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5abf40473ee813c625e0a02232d714aab2e65109
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] threadpool: Allow zero-size task queue
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: threadpool: Allow zero-size task queue
......................................................................
Patch Set 2:
> > curious what the motivation was for this change?
I also updated the commit message to clarify the use case.
--
To view, visit http://gerrit.cloudera.org:8080/5275
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5abf40473ee813c625e0a02232d714aab2e65109
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] threadpool: Allow zero-size task queue
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: threadpool: Allow zero-size task queue
......................................................................
Patch Set 3:
Could you dist-test loop threadpool-test a thousand times or so with --stress_cpu_threads=8 to make sure nothing got racy due to this?
--
To view, visit http://gerrit.cloudera.org:8080/5275
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5abf40473ee813c625e0a02232d714aab2e65109
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] threadpool: Allow zero-size task queue
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5275
to look at the new patch set (#2).
Change subject: threadpool: Allow zero-size task queue
......................................................................
threadpool: Allow zero-size task queue
Previously, our threadpool implementation's concept of max_queue_size
was not very useful, and one could not specify a zero-size queue, because
we took the meaning of queue size quite literally, thus leaking an
implementation detail: we use the task queue to hand off tasks from the
user's thread to our worker threads.
Now, max_queue_size is more intuitive: the user is allowed to submit
(max_queue_size + max_threads) tasks before new submissions are
rejected (assuming no task completes in the mean time).
In this paradigm, a zero-size queue is a useful thing. It implies that
the total number of tasks running or queued at a given time will never
exceed max_threads, which under typical circumstances means that no
successfully-submitted task is left waiting for an executor for very
long.
Added a new functional test for max_queue_size = 0 and updated an
existing queue-related test that is now deterministic instead of being
racy.
Change-Id: I5abf40473ee813c625e0a02232d714aab2e65109
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
2 files changed, 35 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/5275/2
--
To view, visit http://gerrit.cloudera.org:8080/5275
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5abf40473ee813c625e0a02232d714aab2e65109
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] threadpool: Allow zero-size task queue
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: threadpool: Allow zero-size task queue
......................................................................
Patch Set 3: Code-Review-2
-2 for the moment
--
To view, visit http://gerrit.cloudera.org:8080/5275
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5abf40473ee813c625e0a02232d714aab2e65109
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] threadpool: Allow zero-size task queue
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: threadpool: Allow zero-size task queue
......................................................................
threadpool: Allow zero-size task queue
Previously, our threadpool implementation's concept of max_queue_size
was not very useful, and one could not specify a zero-size queue, because
we took the meaning of queue size quite literally, thus leaking an
implementation detail: we use the task queue to hand off tasks from the
user's thread to our worker threads.
Now, max_queue_size is more intuitive: the user is allowed to submit
(max_queue_size + max_threads) tasks before new submissions are
rejected (assuming no task completes in the mean time).
In this paradigm, a zero-size queue is a useful thing. It implies that
the total number of tasks running or queued at a given time will never
exceed max_threads, which under typical circumstances means that no
successfully-submitted task is left waiting for an executor for very
long.
Added a new functional test for max_queue_size = 0 and updated an
existing queue-related test that is now deterministic instead of being
racy.
Change-Id: I5abf40473ee813c625e0a02232d714aab2e65109
Reviewed-on: http://gerrit.cloudera.org:8080/5275
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
2 files changed, 35 insertions(+), 17 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5275
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I5abf40473ee813c625e0a02232d714aab2e65109
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] threadpool: Allow zero-size task queue
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: threadpool: Allow zero-size task queue
......................................................................
Patch Set 3:
TSAN looks good but looping found a few failures in RELEASE mode: http://dist-test.cloudera.org/job?job_id=mpercy.1480541145.24059
I'll look into it.
--
To view, visit http://gerrit.cloudera.org:8080/5275
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5abf40473ee813c625e0a02232d714aab2e65109
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] threadpool: Allow zero-size task queue
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: threadpool: Allow zero-size task queue
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5275
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5abf40473ee813c625e0a02232d714aab2e65109
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No