You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2017/05/06 00:37:54 UTC
[kudu-CR] maintenance manager: schedule work immediately when threads are free
Todd Lipcon has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6815
Change subject: maintenance_manager: schedule work immediately when threads are free
......................................................................
maintenance_manager: schedule work immediately when threads are free
This changes the MM so that, when a worker thread becomes available, it
immediately wakes up the scheduler to schedule the next available work item.
Additionally, the scheduler will loop scheduling new items as long as there
are free worker threads, instead of only scheduling once per polling
interval.
Change-Id: I63c4b48f5f02f3a1d3a8964993e78037ce72b1da
---
M src/kudu/util/maintenance_manager.cc
1 file changed, 12 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/6815/1
--
To view, visit http://gerrit.cloudera.org:8080/6815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I63c4b48f5f02f3a1d3a8964993e78037ce72b1da
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
[kudu-CR] maintenance manager: schedule work immediately when threads are free
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6815
to look at the new patch set (#2).
Change subject: maintenance_manager: schedule work immediately when threads are free
......................................................................
maintenance_manager: schedule work immediately when threads are free
This changes the MM so that, when a worker thread becomes available, it
immediately wakes up the scheduler to schedule the next available work item.
Additionally, the scheduler will loop scheduling new items as long as there
are free worker threads, instead of only scheduling once per polling
interval.
Change-Id: I63c4b48f5f02f3a1d3a8964993e78037ce72b1da
---
M src/kudu/util/maintenance_manager.cc
1 file changed, 21 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/6815/2
--
To view, visit http://gerrit.cloudera.org:8080/6815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I63c4b48f5f02f3a1d3a8964993e78037ce72b1da
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] maintenance manager: schedule work immediately when threads are free
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: maintenance_manager: schedule work immediately when threads are free
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/6815/1/src/kudu/util/maintenance_manager.cc
File src/kudu/util/maintenance_manager.cc:
PS1, Line 43: DEFINE_int32(maintenance_manager_num_threads, 1,
: "Size of the maintenance manager thread pool. "
: "For spinning disks, the number of threads should "
: "not be above the number of devices.");
> should we change this default now?
I don't think so. If anything, this change should make it so that people need fewer MM threads to do the same amount of work, rather than more.
PS1, Line 221: or it is time to run another op.
> The "shutting down" part is fairly intuitive, but this part isn't. It might
Done. Also renamed the var, maybe better this time.
PS1, Line 231: if (!FLAGS_enable_maintenance_manager) {
: KLOG_EVERY_N_SECS(INFO, 30) << "Maintenance manager is disabled. Doing nothing";
: return;
: }
> not your fault but maybe move this above the inner while loop (before LOC 2
Done
PS1, Line 238: // If we found no work to do, then we should sleep before trying again to schedule.
: // Otherwise, we can go right into trying to find the next op.
: force_sleep_next_iter = (op == nullptr);
> since we're already checking whether op is null below maybe it would be cle
but then we also need to set it to false after the if(){} block, right?
--
To view, visit http://gerrit.cloudera.org:8080/6815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I63c4b48f5f02f3a1d3a8964993e78037ce72b1da
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] maintenance manager: schedule work immediately when threads are free
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: maintenance_manager: schedule work immediately when threads are free
......................................................................
maintenance_manager: schedule work immediately when threads are free
This changes the MM so that, when a worker thread becomes available, it
immediately wakes up the scheduler to schedule the next available work item.
Additionally, the scheduler will loop scheduling new items as long as there
are free worker threads, instead of only scheduling once per polling
interval.
Change-Id: I63c4b48f5f02f3a1d3a8964993e78037ce72b1da
Reviewed-on: http://gerrit.cloudera.org:8080/6815
Reviewed-by: David Ribeiro Alves <da...@gmail.com>
Tested-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/util/maintenance_manager.cc
1 file changed, 21 insertions(+), 7 deletions(-)
Approvals:
David Ribeiro Alves: Looks good to me, approved
Todd Lipcon: Verified
--
To view, visit http://gerrit.cloudera.org:8080/6815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I63c4b48f5f02f3a1d3a8964993e78037ce72b1da
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] maintenance manager: schedule work immediately when threads are free
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: maintenance_manager: schedule work immediately when threads are free
......................................................................
Patch Set 2: Verified+1
Unrelated known flake
--
To view, visit http://gerrit.cloudera.org:8080/6815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I63c4b48f5f02f3a1d3a8964993e78037ce72b1da
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] maintenance manager: schedule work immediately when threads are free
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: maintenance_manager: schedule work immediately when threads are free
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I63c4b48f5f02f3a1d3a8964993e78037ce72b1da
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] maintenance manager: schedule work immediately when threads are free
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: maintenance_manager: schedule work immediately when threads are free
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6815/1/src/kudu/util/maintenance_manager.cc
File src/kudu/util/maintenance_manager.cc:
PS1, Line 221: or it is time to run another op.
The "shutting down" part is fairly intuitive, but this part isn't. It might be useful to expand on this and explain how it relates to the two parts of the condition (comparing running_ops to num_threads and force_sleep_next_iter).
--
To view, visit http://gerrit.cloudera.org:8080/6815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I63c4b48f5f02f3a1d3a8964993e78037ce72b1da
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] maintenance manager: schedule work immediately when threads are free
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: maintenance_manager: schedule work immediately when threads are free
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/6815/1/src/kudu/util/maintenance_manager.cc
File src/kudu/util/maintenance_manager.cc:
PS1, Line 43: DEFINE_int32(maintenance_manager_num_threads, 1,
: "Size of the maintenance manager thread pool. "
: "For spinning disks, the number of threads should "
: "not be above the number of devices.");
should we change this default now?
PS1, Line 231: if (!FLAGS_enable_maintenance_manager) {
: KLOG_EVERY_N_SECS(INFO, 30) << "Maintenance manager is disabled. Doing nothing";
: return;
: }
not your fault but maybe move this above the inner while loop (before LOC 221)?
PS1, Line 238: // If we found no work to do, then we should sleep before trying again to schedule.
: // Otherwise, we can go right into trying to find the next op.
: force_sleep_next_iter = (op == nullptr);
since we're already checking whether op is null below maybe it would be clearer to have:
if (!op) {
VLOG_AND_TRACE("maintenance", 2) << LogPrefix()
<< "No maintenance operations look worth doing.";
// If we found no work to do, then we should sleep before trying again to schedule.
force_sleep_next_iter = true;
continue;
}
--
To view, visit http://gerrit.cloudera.org:8080/6815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I63c4b48f5f02f3a1d3a8964993e78037ce72b1da
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes