You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Yuqi Du (Code Review)" <ge...@cloudera.org> on 2022/04/25 10:01:29 UTC
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Yuqi Du has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18447
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 235 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/1
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 1
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Khazar Mammadli, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#17).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support
executing tasks asynchronously and with a delay
I plan using the feature to add tools to trigger data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refactor into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 246 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/17
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 17
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Khazar Mammadli, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#15).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support
executing tasks asynchronously and with a delay
I plan using the feature to add tools to trigger data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refactor into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 246 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/15
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 15
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Khazar Mammadli, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#16).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support
executing tasks asynchronously and with a delay
I plan using the feature to add tools to trigger data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refactor into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 246 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/16
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 16
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Yingchun Lai, Kudu Jenkins, Khazar Mammadli, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#23).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support
executing tasks asynchronously and with a delay
I plan using the feature to add tools to trigger data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refactor into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 257 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/23
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 23
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Khazar Mammadli (Code Review)" <ge...@cloudera.org>.
Khazar Mammadli has posted comments on this change. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18447/11//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18447/11//COMMIT_MSG@9
PS11, Line 9: Add TimerThread to ThreadPool to support to execute tasks
nit: to support executing tasks asynchronously or with a delay
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 11
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Fri, 06 May 2022 09:31:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
Patch Set 20:
(6 comments)
Thanks for you crs.
http://gerrit.cloudera.org:8080/#/c/18447/19/src/kudu/util/threadpool.h
File src/kudu/util/threadpool.h:
http://gerrit.cloudera.org:8080/#/c/18447/19/src/kudu/util/threadpool.h@172
PS19, Line 172: bool empty() const {
> nit: delare it as a const function.
Done
http://gerrit.cloudera.org:8080/#/c/18447/19/src/kudu/util/threadpool.h@280
PS19, Line 280: // isn't set, returns 'false'. Otherwise, returns whether the queue is
> If it is only used in tests, better to deaclare it as a private function.
Done
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc@141
PS17, Line 141: while (!shutdown_.WaitFor(MonoDelta::FromMilliseconds(schedule_period_ms_))) {
> So, is upper_bound OK?
upper_bound is ok. Has fixed.
http://gerrit.cloudera.org:8080/#/c/18447/19/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/18447/19/src/kudu/util/threadpool.cc@127
PS19, Line 127: hedule
> nit: can be omit.
Done
http://gerrit.cloudera.org:8080/#/c/18447/19/src/kudu/util/threadpool.cc@128
PS19, Line 128:
> nit: 'scheduler' is OK, to keep the same style to other Threads.
Done
http://gerrit.cloudera.org:8080/#/c/18447/19/src/kudu/util/threadpool.cc@142
PS19, Line 142: MonoTime now = MonoTime::Now();
> nit: `using std::vector;` in the header of the file and use vector directly
Done
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 20
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Tue, 17 May 2022 03:38:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Khazar Mammadli,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#13).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support
executing tasks asynchronously and with a delay
I plan using the feature to add tools to trigge data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refact into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 250 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/13
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 13
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
Patch Set 26:
(4 comments)
Thanks your crs.
I am sorry lossing the jira message, I reply it now.
http://gerrit.cloudera.org:8080/#/c/18447/26//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18447/26//COMMIT_MSG@9
PS26, Line 9: TimerThread
> Should this be updated to 'SchedulerThread'?
Sorry, I forgot to change it and now it's hard to change because it has been committed to the repository
http://gerrit.cloudera.org:8080/#/c/18447/26/src/kudu/util/threadpool.h
File src/kudu/util/threadpool.h:
http://gerrit.cloudera.org:8080/#/c/18447/26/src/kudu/util/threadpool.h@474
PS26, Line 474: WaitForScheduler
> Is this only for tests?
Now, it's used by tests.
But it can be used by module codes if needed.
http://gerrit.cloudera.org:8080/#/c/18447/26/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/18447/26/src/kudu/util/threadpool.cc@60
PS26, Line 60: 100
> Why the default interval is 100ms, maybe add a comment to explain the reaso
It is used when enable_scheduler_ is true.
SchedulerThread's schedule_period_ms_ should be setted at construct function, so the variable is configurable.
Comments can add later.
http://gerrit.cloudera.org:8080/#/c/18447/26/src/kudu/util/threadpool.cc@250
PS26, Line 250: delay_ms
> What if `delay_ms` is less than or equal to `schedule_period_ms_`? Will the
schedule_period_ms_ is a time precise.
If delay_ms <= schedule_period_ms_, the task will be scheduled at next schedule.
Because of the time precise, the wait time is not a expected time, the wait time is a value in [0, schedule_period_ms_)
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 26
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Wed, 01 Jun 2022 03:13:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#3).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support to execute tasks
asynchronously or task delay execute.
I plan using the feature to add tools to trigge data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refact into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 242 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/3
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 3
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
Patch Set 15:
(1 comment)
Thank you.
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.cc@641
PS13, Line 641: til(const MonoTime& un
> nit: if this is the only usage of task_size_, maybe get rid of it and inste
the task_size used an atomic<int> because of data race.
If we use tasks.empty(), we should use a lock, that's a little heavy.
I can fix it use a lock.
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 15
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Sat, 07 May 2022 11:29:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#11).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support to execute tasks
asynchronously or task delay execute.
I plan using the feature to add tools to trigge data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refact into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 251 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/11
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 11
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
Patch Set 14:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/18447/13//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18447/13//COMMIT_MSG@12
PS13, Line 12: trigge
> nit: trigger
Done
http://gerrit.cloudera.org:8080/#/c/18447/13//COMMIT_MSG@14
PS13, Line 14: refact
> nit: refactor
Done
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h
File src/kudu/util/threadpool.h:
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@126
PS13, Line 126: enable_sched
> nit: how about "enable_scheduling" or somesuch
Done
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@127
PS13, Line 127: scheduler_
> nit: precise_ms isn't particularly descriptive. How about "set_scheduler_pe
Done
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@150
PS13, Line 150: ss Sched
> nit: maybe I'm missing something -- I don't see any periodic scheduling hap
Yes, you are right. I mean it can be used to implement dev/feature/timer_thread.
I' ll fix the comments.
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@151
PS13, Line 151: :
> nit: "timer" makes it seem like this should be measuring the time of tasks.
OK
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@156
PS13, Line 156: // Start a thread to judge which tasks can be execute now.
: Status Start();
:
: // Shutdown the th
> nit: please do your best to describe the behaviors of these methods in comm
Done
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@168
PS13, Line 168: lock(mute
> nit: I think "start_time" would be more descriptive
Done
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@178
PS13, Line 178:
: const std::string thread_pool_name_;
: // timer's period che
> These can be const
Done
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@238
PS13, Line 238: enum class ScheduledTaskWaitType {
: WAIT
> nit: these names are not very descriptive. How about something like
Done.
http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.h
File src/kudu/util/threadpool.h:
http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.h@179
PS11, Line 179: const std::string thread_pool_name_;
> Wouldn't it be better to name this variable period_ms_ (timer_period_ms_) o
Done
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.cc@142
PS13, Line 142: MutexLock auto_lock(mutex_);
: while (!tasks_.empty()) {
: auto it = tasks_.begin();
: if (it->first > now) {
: break;
: }
: ThreadPoolToken* token = it->second.thread_pool_token_;
: CHECK_OK(token->Submit(it->second.f));
: tasks_.erase(it);
: task_size_--;
: }
: }
: }
> nit: I think it'd be a bit more understandable as
Done
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 14
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Sat, 07 May 2022 08:54:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
Patch Set 19:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc@141
PS17, Line 141: MonoTime now = MonoTime::Now();
> I have try to reduce the lock time.
So, is upper_bound OK?
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 19
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Mon, 16 May 2022 12:10:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Yingchun Lai, Kudu Jenkins, Khazar Mammadli, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#24).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support
executing tasks asynchronously and with a delay
I plan using the feature to add tools to trigger data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refactor into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 257 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/24
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 24
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
Patch Set 24: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 24
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Tue, 17 May 2022 12:23:33 +0000
Gerrit-HasComments: No
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
Patch Set 21:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18447/20/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/18447/20/src/kudu/util/threadpool.cc@44
PS20, Line 44: using std::string;
> warning: using decl 'shared_ptr' is unused [misc-unused-using-decls]
Done
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 21
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Tue, 17 May 2022 06:23:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
Patch Set 25: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 25
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Sat, 21 May 2022 15:44:00 +0000
Gerrit-HasComments: No
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support
executing tasks asynchronously and with a delay
I plan using the feature to add tools to trigger data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refactor into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Reviewed-on: http://gerrit.cloudera.org:8080/18447
Tested-by: Kudu Jenkins
Reviewed-by: Yingchun Lai <ac...@gmail.com>
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 257 insertions(+), 5 deletions(-)
Approvals:
Kudu Jenkins: Verified
Yingchun Lai: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 26
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#9).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support to execute tasks
asynchronously or task delay execute.
I plan using the feature to add tools to trigge data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refact into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 240 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/9
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 9
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Yingchun Lai, Kudu Jenkins, Khazar Mammadli, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#21).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support
executing tasks asynchronously and with a delay
I plan using the feature to add tools to trigger data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refactor into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 260 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/21
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 21
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Yingchun Lai, Kudu Jenkins, Khazar Mammadli, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#22).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support
executing tasks asynchronously and with a delay
I plan using the feature to add tools to trigger data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refactor into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 258 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/22
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 22
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
Patch Set 19:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/18447/19/src/kudu/util/threadpool.h
File src/kudu/util/threadpool.h:
http://gerrit.cloudera.org:8080/#/c/18447/19/src/kudu/util/threadpool.h@172
PS19, Line 172: bool empty() {
nit: delare it as a const function.
http://gerrit.cloudera.org:8080/#/c/18447/19/src/kudu/util/threadpool.h@280
PS19, Line 280: // Used by tests to avoid tsan test case down.
If it is only used in tests, better to deaclare it as a private function.
http://gerrit.cloudera.org:8080/#/c/18447/19/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/18447/19/src/kudu/util/threadpool.cc@127
PS19, Line 127: kudu::
nit: can be omit.
http://gerrit.cloudera.org:8080/#/c/18447/19/src/kudu/util/threadpool.cc@128
PS19, Line 128: [scheduler]
nit: 'scheduler' is OK, to keep the same style to other Threads.
http://gerrit.cloudera.org:8080/#/c/18447/19/src/kudu/util/threadpool.cc@142
PS19, Line 142: std::vector<SchedulerTask> pending_tasks;
nit: `using std::vector;` in the header of the file and use vector directly here. Similar to std::string.
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 19
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Mon, 16 May 2022 12:07:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Khazar Mammadli (Code Review)" <ge...@cloudera.org>.
Khazar Mammadli has posted comments on this change. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
Patch Set 11:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.h
File src/kudu/util/threadpool.h:
http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.h@179
PS11, Line 179: // timer's period checking time.
Wouldn't it be better to name this variable period_ms_ (timer_period_ms_) or something similar, so the user could understand the use case of the variable from a glance instead of looking for the specific comment?
http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.h@571
PS11, Line 571: // TimerThread is used for some scenioes, such as
Nit: Scenarios
http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.h@572
PS11, Line 572: // delay execute is schedule,
Nit: scheduled
http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.h@573
PS11, Line 573: // another thread send request.
Comment is rather unclear, could you please clarify it a bit?
http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.h@602
PS11, Line 602: // Submit a task, running after delay_ms delay some time
Does this run the task submitted and delay afterwards or introduces a delay before the task starts the execution? Or is it a periodic delay, aka execute first task--->delay--->execute second task--->delay?
http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.cc@247
PS11, Line 247: return Status::NotSupported(Substitute("not support mode: CONCURRENT"));
nit: Unsupported mode: CONCURRENT
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 11
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Fri, 06 May 2022 09:20:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Khazar Mammadli,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#12).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support to execute tasks
asynchronously or task delay execute.
I plan using the feature to add tools to trigge data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refact into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 253 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/12
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 12
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#6).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support to execute tasks
asynchronously or task delay execute.
I plan using the feature to add tools to trigge data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refact into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 267 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/6
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 6
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/18447/1/src/kudu/util/threadpool.h
File src/kudu/util/threadpool.h:
http://gerrit.cloudera.org:8080/#/c/18447/1/src/kudu/util/threadpool.h@149
PS1, Line 149: class TimerThread {
> warning: single-argument constructors must be marked explicit to avoid unin
Done
http://gerrit.cloudera.org:8080/#/c/18447/1/src/kudu/util/threadpool.h@164
PS1, Line 164: };
> warning: parameter 'f' is passed by value and only copied once; consider mo
Done
http://gerrit.cloudera.org:8080/#/c/18447/1/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/18447/1/src/kudu/util/threadpool.cc@109
PS1, Line 109:
> warning: pass by value and use std::move [modernize-pass-by-value]
Done
http://gerrit.cloudera.org:8080/#/c/18447/1/src/kudu/util/threadpool.cc@110
PS1, Line 110: TimerThread::TimerThread(std::string thread_pool_name)
> warning: parameter 'thread_pool_name' is passed by value and only copied on
Done
http://gerrit.cloudera.org:8080/#/c/18447/1/src/kudu/util/threadpool.cc@249
PS1, Line 249: timepoint.AddDelta(MonoDelta::FromMilliseconds(delay_ms));
> warning: parameter 'f' is passed by value and only copied once; consider mo
Done
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 3
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Mon, 25 Apr 2022 11:31:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
Patch Set 13:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/18447/13//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18447/13//COMMIT_MSG@12
PS13, Line 12: trigge
nit: trigger
http://gerrit.cloudera.org:8080/#/c/18447/13//COMMIT_MSG@14
PS13, Line 14: refact
nit: refactor
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h
File src/kudu/util/threadpool.h:
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@126
PS13, Line 126: enable_timer
nit: how about "enable_scheduling" or somesuch
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@127
PS13, Line 127: precise_ms
nit: precise_ms isn't particularly descriptive. How about "set_scheduler_period_ms" or something
Same for L143, L153, and L180
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@150
PS13, Line 150: periodic
nit: maybe I'm missing something -- I don't see any periodic scheduling happening in this patch.
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@151
PS13, Line 151: TimerThread
nit: "timer" makes it seem like this should be measuring the time of tasks. How about instead calling this SchedulerThread or somesuch?
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@156
PS13, Line 156:
: Status Start();
:
: Status Shutdown();
nit: please do your best to describe the behaviors of these methods in comments
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@168
PS13, Line 168: timepoint
nit: I think "start_time" would be more descriptive
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@178
PS13, Line 178: std::string thread_pool_name_;
: // timer's period checking time.
: uint32_t precise_ms_;
These can be const
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@238
PS13, Line 238: NORMAL,
: TEST
nit: these names are not very descriptive. How about something like
enum class ScheduledTaskWaitType {
WAIT,
NO_WAIT,
};
Though frankly, I'm not sure it's worth adding this enum in general just for the sake of saving a second in one unit test.
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.cc@142
PS13, Line 142: MutexLock auto_lock(mutex_);
: while (true) {
: const auto it = tasks_.begin();
: if (it != tasks_.end() && it->first <= now) {
: ThreadPoolToken* token = it->second.thread_pool_token_;
: CHECK_OK(token->Submit(it->second.f));
: tasks_.erase(it);
: task_size_--;
: } else {
: break;
: }
: }
: }
nit: I think it'd be a bit more understandable as
MutexLock l(mutex_);
while (!tasks_.empty()) {
auto it = tasks_.begin();
if (it->first > now) {
break;
}
ThreadPoolToken* token = it->second.thread_pool_token_;
CHECK_OK(token->Submit(it->second.f));
tasks_.erase(it);
task_size_--;
}
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.cc@246
PS13, Line 246: if (mode() != ThreadPool::ExecutionMode::SERIAL) {
: return Status::NotSupported(Substitute("unsupported mode: CONCURRENT"));
: }
nit: this isn't behavior we ever expect to reach in production -- it is a bug to write code that calls Schedule() when not set to SERIAL. So, let's use a CHECK or DCHECK instead.
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.cc@641
PS13, Line 641: timer_->task_size_ > 0
nit: if this is the only usage of task_size_, maybe get rid of it and instead just use !tasks_.empty()
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 13
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Sat, 07 May 2022 06:50:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Yingchun Lai, Kudu Jenkins, Khazar Mammadli, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#18).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support
executing tasks asynchronously and with a delay
I plan using the feature to add tools to trigger data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refactor into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 259 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/18
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 18
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
Patch Set 17:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.h
File src/kudu/util/threadpool.h:
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.h@126
PS17, Line 126: ThreadPoolBuilder& set_enable_scheduler(bool enable_scheduler);
The default value is false, `set_enable_scheduler` to true implicit, the parameter is not needed?
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.h@127
PS17, Line 127: set_scheduler_period_ms
nit: schedule_period_ms?
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.h@187
PS17, Line 187: mutable Mutex mutex_;
Nit: add comments to describe which variables 'mutex_' to protect.
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.h@579
PS17, Line 579: uint32_t scheduler_period_ms_;
nit: schedule_period_ms_ ?
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc@114
PS17, Line 114: std::string
nit: string
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc@141
PS17, Line 141: MutexLock auto_lock(mutex_);
To reduce lock time, you can obtain and erase the tasks at first, then submit them out of the lock.
And maybe multimap.lower_bound maybe used to optimize a little.
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc@243
PS17, Line 243: CHECK(mode() == ThreadPool::ExecutionMode::SERIAL);
nit: CHECK_EQ
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc@393
PS17, Line 393: scheduler_->Start();
Check if it is OK.
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc@633
PS17, Line 633: // set ScheduledTaskWaitType::WAIT at unit tests.
If ScheduledTaskWaitType::WAIT is only used in tests, better to add a new private function for it.
enum class ScheduledTaskWaitType also in private.
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 17
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Tue, 10 May 2022 09:32:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
Patch Set 26:
(4 comments)
Several days ago I left a comment in https://issues.apache.org/jira/browse/KUDU-3364, seems it's missed somehow.
I didn't see the reason why we need a new thread to schedule tasks at a specific time. Maybe a more straightforward way to trigger long-time running tasks is to make asynchronously RPC calls?
http://gerrit.cloudera.org:8080/#/c/18447/26//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18447/26//COMMIT_MSG@9
PS26, Line 9: TimerThread
Should this be updated to 'SchedulerThread'?
http://gerrit.cloudera.org:8080/#/c/18447/26/src/kudu/util/threadpool.h
File src/kudu/util/threadpool.h:
http://gerrit.cloudera.org:8080/#/c/18447/26/src/kudu/util/threadpool.h@474
PS26, Line 474: WaitForScheduler
Is this only for tests?
http://gerrit.cloudera.org:8080/#/c/18447/26/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/18447/26/src/kudu/util/threadpool.cc@60
PS26, Line 60: 100
Why the default interval is 100ms, maybe add a comment to explain the reason or make it configurable?
http://gerrit.cloudera.org:8080/#/c/18447/26/src/kudu/util/threadpool.cc@250
PS26, Line 250: delay_ms
What if `delay_ms` is less than or equal to `schedule_period_ms_`? Will the task be scheduled at an expected time anyway or in some cases it will not be scheduled at all?
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 26
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Tue, 24 May 2022 08:58:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Yingchun Lai, Kudu Jenkins, Khazar Mammadli, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#19).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support
executing tasks asynchronously and with a delay
I plan using the feature to add tools to trigger data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refactor into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 259 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/19
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 19
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
Patch Set 13:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/18447/11//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18447/11//COMMIT_MSG@9
PS11, Line 9: Add TimerThread to ThreadPool to support
> nit: to support executing tasks asynchronously or with a delay
Done
http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.h
File src/kudu/util/threadpool.h:
http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.h@571
PS11, Line 571: // TimerThread is used for some scenarios, such as
> Nit: Scenarios
Done
http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.h@572
PS11, Line 572: // make a task delay execution.
> Nit: scheduled
Done
http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.h@573
PS11, Line 573: TimerThread* timer_;
> Comment is rather unclear, could you please clarify it a bit?
Done
http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.h@602
PS11, Line 602: Status Schedule(std::function<void()> f, int64_t delay_ms) WARN_UNUSED_RESULT;
> Does this run the task submitted and delay afterwards or introduces a delay
delay---> execute task.
http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.cc@247
PS11, Line 247: return Status::NotSupported(Substitute("unsupported mode: CONCURRENT"));
> nit: Unsupported mode: CONCURRENT
Done
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 13
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Sat, 07 May 2022 02:57:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Khazar Mammadli, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#14).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support
executing tasks asynchronously and with a delay
I plan using the feature to add tools to trigger data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refactor into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 245 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/14
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 14
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
Patch Set 18:
(9 comments)
Thanks your crs.
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.h
File src/kudu/util/threadpool.h:
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.h@126
PS17, Line 126: ThreadPoolBuilder& set_enable_scheduler();
> The default value is false, `set_enable_scheduler` to true implicit, the pa
Done
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.h@127
PS17, Line 127: chedule_period_ms);
> nit: schedule_period_ms?
Done
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.h@187
PS17, Line 187: // Protect `tasks_` data race.
> Nit: add comments to describe which variables 'mutex_' to protect.
Done
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.h@579
PS17, Line 579: uint32_t schedule_period_ms_;
> nit: schedule_period_ms_ ?
Done
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc@114
PS17, Line 114:
> nit: string
Done
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc@141
PS17, Line 141: MonoTime now = MonoTime::Now();
> To reduce lock time, you can obtain and erase the tasks at first, then subm
I have try to reduce the lock time.
lower_bound maybe not good, because task equal MonoTime should be executed, the bound may exclude some tasks.
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc@243
PS17, Line 243: }
> nit: CHECK_EQ
CHECK_EQ not ok. ThreadPool::ExecutionMode should printable, but no implements.
So reserve it.
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc@393
PS17, Line 393: Status status = CreateThread();
> Check if it is OK.
Done
http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc@633
PS17, Line 633:
> If ScheduledTaskWaitType::WAIT is only used in tests, better to add a new p
Done
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 18
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Wed, 11 May 2022 09:04:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Yingchun Lai, Kudu Jenkins, Khazar Mammadli, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#20).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support
executing tasks asynchronously and with a delay
I plan using the feature to add tools to trigger data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refactor into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 260 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/20
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 20
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Yingchun Lai, Kudu Jenkins, Khazar Mammadli, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#25).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support
executing tasks asynchronously and with a delay
I plan using the feature to add tools to trigger data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refactor into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 257 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/25
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 25
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#2).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support to execute tasks
asynchronously or task delay execute.
I plan using the feature to add tools to trigge data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refact into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 235 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/2
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 2
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
Patch Set 25:
> Patch Set 25: Code-Review+2
It's very kind of you.
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 25
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Sun, 22 May 2022 13:37:57 +0000
Gerrit-HasComments: No
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#4).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support to execute tasks
asynchronously or task delay execute.
I plan using the feature to add tools to trigge data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refact into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 243 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/4
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 4
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#5).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support to execute tasks
asynchronously or task delay execute.
I plan using the feature to add tools to trigge data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refact into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 267 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/5
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 5
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18447
to look at the new patch set (#7).
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
......................................................................
[threadpool] KUDU-3364 Add TimerThread for thread pool
Add TimerThread to ThreadPool to support to execute tasks
asynchronously or task delay execute.
I plan using the feature to add tools to trigge data rebalance,
leader rebalance, some kinds of compaction ops. Of course, some
single threads can refact into one threadpool.
More details at KUDU-3364.
Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 242 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18447/7
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 7
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>