You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2020/08/12 23:04:51 UTC
[kudu-CR] WIP KUDU-1587 part 1: load meter for ThreadPool
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16332
Change subject: WIP KUDU-1587 part 1: load meter for ThreadPool
......................................................................
WIP KUDU-1587 part 1: load meter for ThreadPool
This patch introduces a load meter for ThreadPool, aiming to use
active queue management techniques (AQM) such as CoDel [1] in scenarios
where thread pool queue load metrics are applicable (e.g., KUDU-1587).
WIP:
* collect initial feedback
* add unit tests
[1] https://en.wikipedia.org/wiki/CoDel
Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
---
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
2 files changed, 231 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/16332/1
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16332
to look at the new patch set (#14).
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
KUDU-1587 part 1: load meter for ThreadPool
This patch introduces a load meter for ThreadPool, aiming to use
active queue management techniques (AQM) such as CoDel [1] in scenarios
where thread pool queue load metrics are applicable (e.g., KUDU-1587).
[1] https://en.wikipedia.org/wiki/CoDel
Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 602 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/16332/14
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 14
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Patch Set 3:
(9 comments)
Thanks for the heads up. I'd already started reviewing, so I'm posting what I had.
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.h
File src/kudu/util/threadpool.h:
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.h@75
PS2, Line 75: queue_time_history_length
nit: typo
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.h@79
PS2, Line 79: set_queue_time_history_length
Where is this defined?
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.h@99
PS2, Line 99: // the information on the next task's submit time is specified
: // via the 'queue_head_submit_time' parameter.
nit: can you also mention that it is OK for queue_head_submit_time to be uninitialized, e.g. if there are no tasks left in the queue
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.h@108
PS2, Line 108: overloaded
nit: unfinished sentence?
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.cc@70
PS2, Line 70: return true;
nit: update these to be actual messages
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.cc@132
PS2, Line 132: }
:
: voi
Why don't we want to call this if 'queue_head_submit_time' is uninitialized? Isn't that how we determine that there is nothing in the queue?
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.cc@138
PS2, Line 138: has_idle_thr
nit: also suffix this and TaskDequeued() with Unlocked?
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.cc@710
PS2, Line 710:
nit: annotate with /*has_idle_threads*/
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.cc@756
PS2, Line 756: // but for tokens of the SERIAL execution mode, the token isn't in the
: // queue_, but will be put there after executing current task.
: if (!queue_.empty() && !queue_.front()->entries_.empty()) {
: head_token = queue_.front();
: } else if (!token->entries_.empty()) {
: head_token = token;
: }
: const MonoTime now(MonoTime::Now());
: const MonoDelta queue_time = now - task.submit_time;
: if (metrics_.load_meter) {
:
Can we move this into the if(load_meter) block?
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 14 Aug 2020 19:36:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 14
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16332
to look at the new patch set (#9).
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
KUDU-1587 part 1: load meter for ThreadPool
This patch introduces a load meter for ThreadPool, aiming to use
active queue management techniques (AQM) such as CoDel [1] in scenarios
where thread pool queue load metrics are applicable (e.g., KUDU-1587).
[1] https://en.wikipedia.org/wiki/CoDel
Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 635 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/16332/9
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 9
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Patch Set 14: Verified+1
unrelated test failure in:
* RaftConsensusITest.TestKUDU_597
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 14
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 27 Aug 2020 22:45:17 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16332/8/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/16332/8/src/kudu/util/threadpool.cc@301
PS8, Line 301: max_threads_
What's the intuition behind using the max number of threads as the overload window? Would it make sense to make this tunable?
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 24 Aug 2020 19:58:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Patch Set 8: Verified+1
unrelated test failures:
* HmsConfigurations/MasterStressTest.Test/1
* RangerClientTestBase.TestLogging
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 24 Aug 2020 18:48:22 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16332
to look at the new patch set (#16).
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
KUDU-1587 part 1: load meter for ThreadPool
This patch introduces a load meter for ThreadPool, aiming to use
active queue management techniques (AQM) such as CoDel [1] in scenarios
where thread pool queue load metrics are applicable (e.g., KUDU-1587).
[1] https://en.wikipedia.org/wiki/CoDel
Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 603 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/16332/16
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 16
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Patch Set 13:
(3 comments)
> (4 comments)
>
> Was mid-review before the new revision, but the comments are still
> valid.
Sorry for the mess. I was trying to remove spurious updates on spare thread availability, and in PS14 it looks simpler now.
http://gerrit.cloudera.org:8080/#/c/16332/13/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/16332/13/src/kudu/util/threadpool.cc@519
PS13, Line 519: if (load_meter_) {
: MonoTime queue_head_submit_time;
: if (!queue_.empty()) {
: DCHECK(!queue_.front()->entries_.empty());
: queue_head_submit_time = queue_.front()->entries_.front().submit_time;
: }
: QueueLoadMeter::SpareThreadAvailability spare_thread_presence =
: QueueLoadMeter::SpareThreadAvailability::UNKNOWN;
: if (!idle_threads_.empty()) {
: spare_thread_presence = QueueLoadMeter::SpareThreadAvailability::PRESENT;
: } else if (max_threads_ == active_threads_) {
: spare_thread_presence = QueueLoadMeter::SpareThreadAvailability::ABSENT;
: }
: load_meter_->UpdateQueueInfoUnlocked(MonoDelta(),
: queue_head_submit_time,
: spare_thread_presence);
: }
> Similar code is used in a few places now. Is it similar enough to encapsula
I simplified these, and in PS14 it's not that much of the code. If talking about PS14, does it still look like a valid concern?
http://gerrit.cloudera.org:8080/#/c/16332/13/src/kudu/util/threadpool.cc@667
PS13, Line 667: load_meter_->UpdateQueueInfoUnlocked(queue_time,
: queue_head_submit_time,
: spare_thread_presence);
> Just making sure we have our bases covered, I think it's correct to say tha
Right, UpdateQueueInfoUnlocked should report about events which affects our criterion to evaluate the state of the queue (overloaded vs normal). These are updates about:
* queue time of a newly dequeued task
* submit time of the task at the head of the queue
* change in the availability of spare working threads
It means we want to call UpdateQueueInfoUnlocked() at each of the following events:
* the task at the head of the queue dispatched to be run
* a new task scheduled (i.e. added into the queue)
* a worker thread completes running a task
http://gerrit.cloudera.org:8080/#/c/16332/13/src/kudu/util/threadpool.cc@747
PS13, Line 747: max_threads_ == active_threads_
> Why don't we have to wait to decrement active_threads_ below?
This outer 'else' cause goes next cycle of the outer while loop and picks up a next task immediately, incrementing active_threads_ back (while holding lock).
Instead, the idea was to avoid such spurious reports.
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 13
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 28 Aug 2020 00:32:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16332/8/src/kudu/util/threadpool-test.cc
File src/kudu/util/threadpool-test.cc:
http://gerrit.cloudera.org:8080/#/c/16332/8/src/kudu/util/threadpool-test.cc@463
PS8, Line 463: // Test scenario to verify the functionality of the QueueLoadMeter.
: TEST_F(ThreadPoolTest, QueueLoadMeter) {
While the serial case may not be the focus of this series, since it's in this patch, can you add some test coverage for token-based submission?
http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool.cc@809
PS7, Line 809: // threadpool's activity, the queue might have stalled because all its worker
: // threads have been busy for long time. If so, 'overloaded_since_' hasn't
: // been updated by the activity on the thread pool itself. However, it's
: /
> Nope, I don't think it make sense to put this atop of the method.
I see, thanks for explaining! It didn't connect for me at first that the initial check is only checking the overloaded state since the last insertion/removal from the queue.
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 24 Aug 2020 19:34:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Patch Set 13: Verified+1
Unrelated test failure: TxnStatusTableITest.TestSystemClientTServerDown
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 13
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 27 Aug 2020 18:05:31 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16332
to look at the new patch set (#8).
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
KUDU-1587 part 1: load meter for ThreadPool
This patch introduces a load meter for ThreadPool, aiming to use
active queue management techniques (AQM) such as CoDel [1] in scenarios
where thread pool queue load metrics are applicable (e.g., KUDU-1587).
[1] https://en.wikipedia.org/wiki/CoDel
Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 422 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/16332/8
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Patch Set 16: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 16
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 28 Aug 2020 06:02:13 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16332/5/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/16332/5/src/kudu/util/threadpool.cc@86
PS5, Line 86: compare_exchange_weak
typo: this must have been compare_exchange_strong
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 15 Aug 2020 08:01:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16332/3/src/kudu/util/threadpool.h
File src/kudu/util/threadpool.h:
http://gerrit.cloudera.org:8080/#/c/16332/3/src/kudu/util/threadpool.h@154
PS3, Line 154: scoped_refptr<QueueLoadMeter> load_meter;
Once I started implementing the code that uses the newly introduced functionality, I realized it's better to make the load meter an internal class of ThreadPool and introduce a new method ThreadPool::IsQueueOverloaded().
I guess it doesn't change much of the functionality, but just a heads-up.
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 14 Aug 2020 19:17:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16332
to look at the new patch set (#2).
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
KUDU-1587 part 1: load meter for ThreadPool
This patch introduces a load meter for ThreadPool, aiming to use
active queue management techniques (AQM) such as CoDel [1] in scenarios
where thread pool queue load metrics are applicable (e.g., KUDU-1587).
[1] https://en.wikipedia.org/wiki/CoDel
Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 352 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/16332/2
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16332
to look at the new patch set (#4).
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
KUDU-1587 part 1: load meter for ThreadPool
This patch introduces a load meter for ThreadPool, aiming to use
active queue management techniques (AQM) such as CoDel [1] in scenarios
where thread pool queue load metrics are applicable (e.g., KUDU-1587).
[1] https://en.wikipedia.org/wiki/CoDel
Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 390 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/16332/4
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Patch Set 14:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16332/14/src/kudu/util/threadpool.h
File src/kudu/util/threadpool.h:
http://gerrit.cloudera.org:8080/#/c/16332/14/src/kudu/util/threadpool.h@368
PS14, Line 368: spawn
nit: spawned, here and below
http://gerrit.cloudera.org:8080/#/c/16332/13/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/16332/13/src/kudu/util/threadpool.cc@667
PS13, Line 667: nst int64_t queue_time_us = queue_time.ToMicroseconds();
: TRACE_COUNTER_INCREMENT(queue_time_trace_metric_name_, queue_time_us);
: if (metrics_.queue_time_us_histogram) {
> Right, UpdateQueueInfoUnlocked should report about events which affects our
Great, thanks for clarifying. Do you think it's worth mentioning that somewhere in a comment?
http://gerrit.cloudera.org:8080/#/c/16332/14/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/16332/14/src/kudu/util/threadpool.cc@723
PS14, Line 723: load_meter_->UpdateQueueInfoUnlocked(MonoDelta(), MonoTime(), true);
Other call-sites can't assume these arguments just because the queue is empty. But we can here, because not only do we know the queue is empty, but we know there is an available thread (the current thread), also don't have an update to the queue time history. Mind adding a comment here explaining that? Future readers may come upon this code and wonder why the difference in calls.
That said, could we have reused the same code block in all three call-sites?
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 14
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 28 Aug 2020 02:53:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16332/11/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/16332/11/src/kudu/util/threadpool.cc@605
PS11, Line 605: // The uninitialized value of MonoTime for the next task to be run means
: // it's an empty queue, so no task to be run.
> This might be true if we had a guarantee that DispatchThreads() always find
An update: this is necessary to as a notification on spare threads appearing. Without this piece, in some situations, a previously overloaded queue would be reported overloaded even after all tasks are complete.
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 27 Aug 2020 06:29:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP KUDU-1587 part 1: load meter for ThreadPool
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: WIP KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Patch Set 1:
just did a quick skim over the patch, would be good to get someone to review in more detail.
Generally it seems reasonable, I think the concerns would be:
(1) if not enabled, confirm that this doesn't add any noticeable overhead
(2) if enabled, measure how much overhead this adds (eg does the threadpool become noticeably less scalable due to more work happening under the threadpool lock?)
Also might make sense to hold off on committing this until the later patch(es) in the series that make use of it prove out that it effectively solves the KUDU-1587 issue. I had sketched out the "overload detection" solution on the JIRA but I don't know if it will work in practice :)
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 12 Aug 2020 23:09:37 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Patch Set 7:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool-test.cc
File src/kudu/util/threadpool-test.cc:
http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool-test.cc@191
PS7, Line 191: then
> nit: than
Done
http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool.h
File src/kudu/util/threadpool.h:
http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool.h@336
PS7, Line 336: bool
> Does this need to be atomic? If not, maybe mention that this should only be
Good catch: it turned out this is not needed as a part of internal state of the object.
http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool.cc@608
PS7, Line 608: if (PREDICT_FALSE(active_threads_
> Just checking I understand the nuances of why this is important. Even thoug
Right, this 'if' condition is very unlikely to evaluate to 'true' in case of a pool that handles mostly tasks in CONCURRENT execution mode, but in general case it might be more or less regular case. However, 'load_meter_' is used only in case of apply pool, and that's why I added this PREDICT_FALSE(). I agree that it's better to remove PREDICT_FALSE() here because the code doesn't make any explicit assumptions on the way how this functionality is used.
http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool.cc@809
PS7, Line 809: MonoTime queue_head_submit_time = queue_head_submit_time_.load();
: if (!queue_head_submit_time.Initialized()) {
: return false;
: }
> I'm curious why do this _after_ checking whether the meter has been deemed
Nope, I don't think it make sense to put this atop of the method.
The first check is to see whether the queue was overloaded upon the last update on the queue from the threadpool's activity. If that's the case, there is no need to dig further.
However, there might be no updates on the queue for a long time, and the task in the head of the queue might be sitting there for longer than the configured queue_time_threshold_ interval.
There is an explanation in the comment above, maybe it's not clear enough.
I'll add more comments; hopefully it's clearer now.
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 24 Aug 2020 17:39:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16332/8/src/kudu/util/threadpool-test.cc
File src/kudu/util/threadpool-test.cc:
http://gerrit.cloudera.org:8080/#/c/16332/8/src/kudu/util/threadpool-test.cc@463
PS8, Line 463: // Test scenario to verify the functionality of the QueueLoadMeter.
: TEST_F(ThreadPoolTest, QueueLoadMeter) {
> While the serial case may not be the focus of this series, since it's in th
Good call. Done.
http://gerrit.cloudera.org:8080/#/c/16332/8/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/16332/8/src/kudu/util/threadpool.cc@301
PS8, Line 301: max_threads_
> What's the intuition behind using the max number of threads as the overload
Nope, I don't think we need to have some tunable parameter for that. I think max_threads gives us the proper setting for the history size given our criterion on the overloaded state (where N is the size of the queue time history):
max(QT_head, min(QT_historic(N))) > T_overload
I added an extra paragraph about this into the class-wide comment for QueueLoadMeter.
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 25 Aug 2020 21:20:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16332
to look at the new patch set (#6).
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
KUDU-1587 part 1: load meter for ThreadPool
This patch introduces a load meter for ThreadPool, aiming to use
active queue management techniques (AQM) such as CoDel [1] in scenarios
where thread pool queue load metrics are applicable (e.g., KUDU-1587).
[1] https://en.wikipedia.org/wiki/CoDel
Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 394 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/16332/6
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Patch Set 7:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool-test.cc
File src/kudu/util/threadpool-test.cc:
http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool-test.cc@191
PS7, Line 191: then
nit: than
http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool.h
File src/kudu/util/threadpool.h:
http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool.h@336
PS7, Line 336: bool
Does this need to be atomic? If not, maybe mention that this should only be accessed when the threadpool's lock is held?
http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool.cc@608
PS7, Line 608: if (PREDICT_FALSE(active_threads_
Just checking I understand the nuances of why this is important. Even though the queue may be empty, another thread may be processing work from a currently-running token, which isn't reflected by the queue's emptiness.
Therefore, we need to check the active thread count before determining that there is indeed no work currently being processed. Otherwise, more entries will be dequeued from the other thread's token which would update the queue overloading info, but before doing so, we'd incorrectly have temporarily marked the queue as not overloaded.
If so, I'm curious just how rare that actually is (i.e. why PREDICT_FALSE?). It seems like the big places we use SERIAL are:
- in each tablet's prepare pool token
- in each tablet's Raft pool token
Why is it more likely that the number of active threads will be zero when the queue is empty? I may be missing something, but If there are many tokens doing work concurrently, I'd think the number active threads upon emptying 'queue_' would be non-zero a lot of the time.
http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool.cc@809
PS7, Line 809: MonoTime queue_head_submit_time = queue_head_submit_time_.load();
: if (!queue_head_submit_time.Initialized()) {
: return false;
: }
I'm curious why do this _after_ checking whether the meter has been deemed not previously overloaded. Would it make sense to put this at the top of the method?
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 17 Aug 2020 23:35:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Patch Set 11:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16332/11/src/kudu/util/threadpool-test.cc
File src/kudu/util/threadpool-test.cc:
http://gerrit.cloudera.org:8080/#/c/16332/11/src/kudu/util/threadpool-test.cc@569
PS11, Line 569: SleepFor(MonoDelta::FromMilliseconds(10));
> What's the rationale behind the different low-value sleeps?
I guess this is a remnant from a prior version when the tasks were sleeping for a millisecond. Removed.
http://gerrit.cloudera.org:8080/#/c/16332/11/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/16332/11/src/kudu/util/threadpool.cc@605
PS11, Line 605: // The uninitialized value of MonoTime for the next task to be run means
: // it's an empty queue, so no task to be run.
> Would this have already been called by whatever dequeued the last task? I.e
This might be true if we had a guarantee that DispatchThreads() always finds a item in the queue. I wasn't sure about that, so I added this piece. I'll take a closer look at this.
http://gerrit.cloudera.org:8080/#/c/16332/11/src/kudu/util/threadpool.cc@862
PS11, Line 862: queue_head_submit_time_.store(queue_head_submit_time);
: has_spare_threads_.store(has_spare_threads);
: UpdateOverloadedStateUnlocked();
: VLOG(4) << Substitute("queue info snapshot: { $0, $1, $2 }",
: has_spare_threads,
: queue_head_submit_time_.load().ToString(),
: overloaded_since_.load().ToString());
> nit: calling UpdateQueueInfoUnlocked() here instead would clarify the relat
Good point! Done.
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 26 Aug 2020 20:30:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Patch Set 11:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16332/11/src/kudu/util/threadpool-test.cc
File src/kudu/util/threadpool-test.cc:
http://gerrit.cloudera.org:8080/#/c/16332/11/src/kudu/util/threadpool-test.cc@569
PS11, Line 569: SleepFor(MonoDelta::FromMilliseconds(10));
What's the rationale behind the different low-value sleeps?
http://gerrit.cloudera.org:8080/#/c/16332/11/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/16332/11/src/kudu/util/threadpool.cc@605
PS11, Line 605: // The uninitialized value of MonoTime for the next task to be run means
: // it's an empty queue, so no task to be run.
Would this have already been called by whatever dequeued the last task? I.e. could we move the UpdateQueueInfoUnlocked() call into the if block?
http://gerrit.cloudera.org:8080/#/c/16332/11/src/kudu/util/threadpool.cc@862
PS11, Line 862: queue_head_submit_time_.store(queue_head_submit_time);
: has_spare_threads_.store(has_spare_threads);
: UpdateOverloadedStateUnlocked();
: VLOG(4) << Substitute("queue info snapshot: { $0, $1, $2 }",
: has_spare_threads,
: queue_head_submit_time_.load().ToString(),
: overloaded_since_.load().ToString());
nit: calling UpdateQueueInfoUnlocked() here instead would clarify the relationship between TaskDequeuedUnlocked() and UpdateQueueInfoUnlocked().
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 26 Aug 2020 18:56:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16332
to look at the new patch set (#15).
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
KUDU-1587 part 1: load meter for ThreadPool
This patch introduces a load meter for ThreadPool, aiming to use
active queue management techniques (AQM) such as CoDel [1] in scenarios
where thread pool queue load metrics are applicable (e.g., KUDU-1587).
[1] https://en.wikipedia.org/wiki/CoDel
Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 603 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/16332/15
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 15
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16332
to look at the new patch set (#7).
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
KUDU-1587 part 1: load meter for ThreadPool
This patch introduces a load meter for ThreadPool, aiming to use
active queue management techniques (AQM) such as CoDel [1] in scenarios
where thread pool queue load metrics are applicable (e.g., KUDU-1587).
[1] https://en.wikipedia.org/wiki/CoDel
Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 416 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/16332/7
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16332
to look at the new patch set (#10).
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
KUDU-1587 part 1: load meter for ThreadPool
This patch introduces a load meter for ThreadPool, aiming to use
active queue management techniques (AQM) such as CoDel [1] in scenarios
where thread pool queue load metrics are applicable (e.g., KUDU-1587).
[1] https://en.wikipedia.org/wiki/CoDel
Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 633 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/16332/10
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 10
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16332
to look at the new patch set (#12).
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
KUDU-1587 part 1: load meter for ThreadPool
This patch introduces a load meter for ThreadPool, aiming to use
active queue management techniques (AQM) such as CoDel [1] in scenarios
where thread pool queue load metrics are applicable (e.g., KUDU-1587).
[1] https://en.wikipedia.org/wiki/CoDel
Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 602 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/16332/12
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 12
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Patch Set 2:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.h
File src/kudu/util/threadpool.h:
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.h@75
PS2, Line 75: queue_time_history_lenght
> nit: typo
Done
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.h@79
PS2, Line 79: set_queue_time_history_length
> Where is this defined?
This has gone. I updated the comment.
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.h@99
PS2, Line 99: // the information on the next task's submit time is specified
: // via the 'queue_head_submit_time' parameter.
> nit: can you also mention that it is OK for queue_head_submit_time to be un
Done
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.h@108
PS2, Line 108: overloaded
> nit: unfinished sentence?
Done
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.cc@70
PS2, Line 70: VLOG(4) << Substitute("XXX");
> nit: update these to be actual messages
Done
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.cc@132
PS2, Line 132: if (queue_head_submit_time.Initialized()) {
: queue_head_submit_time_ = queue_head_submit_time;
: }
> Why don't we want to call this if 'queue_head_submit_time' is uninitialized
Ah, indeed. It seems I changed the semantics of this in-the-middle, and that's the old one.
Good catch!
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.cc@138
PS2, Line 138: UpdateQueueInfo
> nit: also suffix this and TaskDequeued() with Unlocked?
Done
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.cc@710
PS2, Line 710: true
> nit: annotate with /*has_idle_threads*/
Done
http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.cc@756
PS2, Line 756: // Prepare information for load_meter.
: ThreadPoolToken* head_token = nullptr;
: // Find the element to be dispatched next. For tokens with execution mode
: // CONCURRENT, the next task is referred from a token in the head of queue_,
: // but for tokens of the SERIAL execution mode, the token isn't in the
: // queue_, but will be put there after executing current task.
: if (!queue_.empty() && !queue_.front()->entries_.empty()) {
: head_token = queue_.front();
: } else if (!token->entries_.empty()) {
: head_token = token;
: }
> Can we move this into the if(load_meter) block?
Done
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 15 Aug 2020 02:41:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16332
to look at the new patch set (#3).
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
KUDU-1587 part 1: load meter for ThreadPool
This patch introduces a load meter for ThreadPool, aiming to use
active queue management techniques (AQM) such as CoDel [1] in scenarios
where thread pool queue load metrics are applicable (e.g., KUDU-1587).
[1] https://en.wikipedia.org/wiki/CoDel
Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 364 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/16332/3
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
KUDU-1587 part 1: load meter for ThreadPool
This patch introduces a load meter for ThreadPool, aiming to use
active queue management techniques (AQM) such as CoDel [1] in scenarios
where thread pool queue load metrics are applicable (e.g., KUDU-1587).
[1] https://en.wikipedia.org/wiki/CoDel
Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Reviewed-on: http://gerrit.cloudera.org:8080/16332
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 603 insertions(+), 28 deletions(-)
Approvals:
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 17
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 13
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
Patch Set 14:
(4 comments)
Was mid-review before the new revision, but the comments are still valid.
http://gerrit.cloudera.org:8080/#/c/16332/13/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/16332/13/src/kudu/util/threadpool.cc@519
PS13, Line 519: if (load_meter_) {
: MonoTime queue_head_submit_time;
: if (!queue_.empty()) {
: DCHECK(!queue_.front()->entries_.empty());
: queue_head_submit_time = queue_.front()->entries_.front().submit_time;
: }
: load_meter_->UpdateQueueInfoUnlocked(MonoDelta(),
: queue_head_submit_time,
: active_threads_ < max_threads_);
: }
: guard.Unlock();
:
: if (metrics_.queue_length_histogram) {
: metrics_.queue_length_histogram->Increment(length_at_submit);
: }
: if (token->metrics_.queue_length_histogram) {
:
Similar code is used in a few places now. Is it similar enough to encapsulate and reuse?
http://gerrit.cloudera.org:8080/#/c/16332/13/src/kudu/util/threadpool.cc@667
PS13, Line 667: nst int64_t queue_time_us = queue_time.ToMicroseconds();
: TRACE_COUNTER_INCREMENT(queue_time_trace_metric_name_, queue_time_us);
: if (metrics_.queue_time_us_histogram) {
Just making sure we have our bases covered, I think it's correct to say that we should call this whenever:
- we dequeue/start a task, since we will end up with a new head (and thus a new queue head submit time) _and_ a new queue time to add to our history,
- we complete a task, since we may end up with a new view of whether or not we have available threads,
- we enqueue a new task, since this may be only item on the queue, and thus, may yield a new queue head submit time.
Is that correct?
http://gerrit.cloudera.org:8080/#/c/16332/13/src/kudu/util/threadpool.cc@747
PS13, Line 747:
Why don't we have to wait to decrement active_threads_ below?
http://gerrit.cloudera.org:8080/#/c/16332/13/src/kudu/util/threadpool.cc@885
PS13, Line 885: std::ostream& operator<<(std::ostream& o, ThreadPoolToken::State s
Now that there is only a single caller, how about inlining this call instead?
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 14
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 27 Aug 2020 22:53:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1587 part 1: load meter for ThreadPool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16332
to look at the new patch set (#13).
Change subject: KUDU-1587 part 1: load meter for ThreadPool
......................................................................
KUDU-1587 part 1: load meter for ThreadPool
This patch introduces a load meter for ThreadPool, aiming to use
active queue management techniques (AQM) such as CoDel [1] in scenarios
where thread pool queue load metrics are applicable (e.g., KUDU-1587).
[1] https://en.wikipedia.org/wiki/CoDel
Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 642 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/16332/13
--
To view, visit http://gerrit.cloudera.org:8080/16332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b
Gerrit-Change-Number: 16332
Gerrit-PatchSet: 13
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>