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 2016/09/02 00:13:00 UTC
[kudu-CR] KUDU-1495. Maintenance manager should not schedule new ops during unregister
Hello Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4295
to review the following change.
Change subject: KUDU-1495. Maintenance manager should not schedule new ops during unregister
......................................................................
KUDU-1495. Maintenance manager should not schedule new ops during unregister
This fixes a bug where deleting a tablet could block for an extended
time on compactions being scheduled if the maintenance manager was
configured with multiple threads.
The issue was that Unregister() waited for the number of running
operations to reach 0 before removing the op from the candidate list of
ops, but did not prevent new instances of that op from being scheduled.
Change-Id: I3675705caf5b73f8a480036b974e4db6c205616a
---
M src/kudu/tablet/tablet.cc
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
4 files changed, 74 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/4295/1
--
To view, visit http://gerrit.cloudera.org:8080/4295
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3675705caf5b73f8a480036b974e4db6c205616a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
[kudu-CR] KUDU-1495. Maintenance manager should not schedule new ops during unregister
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1495. Maintenance manager should not schedule new ops during unregister
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4295/1/src/kudu/util/maintenance_manager-test.cc
File src/kudu/util/maintenance_manager-test.cc:
Line 209: });
> hrm, really? this is how my emacs indents it, for better or worse... you th
Nah, if it's more consistent this way then keep it as-is.
http://gerrit.cloudera.org:8080/#/c/4295/1/src/kudu/util/maintenance_manager.h
File src/kudu/util/maintenance_manager.h:
Line 190: void Cancel() {
> was hoping that we can add some polling from within the ops themselves at s
I see. Perhaps CancelAndDisable() or CancelAndStop() then?
My beef with Cancel() as-is is just that it doesn't suggest the end of events in the future. To use a dumb metaphor, "the game was cancelled" doesn't imply that there won't be more games in the future.
--
To view, visit http://gerrit.cloudera.org:8080/4295
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675705caf5b73f8a480036b974e4db6c205616a
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-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1495. Maintenance manager should not schedule new ops during unregister
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: KUDU-1495. Maintenance manager should not schedule new ops during unregister
......................................................................
KUDU-1495. Maintenance manager should not schedule new ops during unregister
This fixes a bug where deleting a tablet could block for an extended
time on compactions being scheduled if the maintenance manager was
configured with multiple threads.
The issue was that Unregister() waited for the number of running
operations to reach 0 before removing the op from the candidate list of
ops, but did not prevent new instances of that op from being scheduled.
Change-Id: I3675705caf5b73f8a480036b974e4db6c205616a
Reviewed-on: http://gerrit.cloudera.org:8080/4295
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/tablet/tablet.cc
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
4 files changed, 74 insertions(+), 7 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4295
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I3675705caf5b73f8a480036b974e4db6c205616a
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1495. Maintenance manager should not schedule new ops during unregister
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1495. Maintenance manager should not schedule new ops during unregister
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4295/1/src/kudu/util/maintenance_manager-test.cc
File src/kudu/util/maintenance_manager-test.cc:
Line 197: TEST_F(MaintenanceManagerTest, TestNewOpsDontGetScheduledDuringUnregister) {
This test failed without the fix, right?
Line 209: });
Nit: indentation.
http://gerrit.cloudera.org:8080/#/c/4295/1/src/kudu/util/maintenance_manager.h
File src/kudu/util/maintenance_manager.h:
Line 190: void Cancel() {
Hmm. Cancel makes it sound like we're canceling a discrete run of the operation, but we're actually making sure there won't be any future runs. Maybe Disable() and disabled() instead?
--
To view, visit http://gerrit.cloudera.org:8080/4295
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675705caf5b73f8a480036b974e4db6c205616a
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] KUDU-1495. Maintenance manager should not schedule new ops during unregister
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1495. Maintenance manager should not schedule new ops during unregister
......................................................................
Patch Set 1:
Build Started http://104.196.14.100/job/kudu-gerrit/3196/
--
To view, visit http://gerrit.cloudera.org:8080/4295
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675705caf5b73f8a480036b974e4db6c205616a
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: No
[kudu-CR] KUDU-1495. Maintenance manager should not schedule new ops during unregister
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4295
to look at the new patch set (#2).
Change subject: KUDU-1495. Maintenance manager should not schedule new ops during unregister
......................................................................
KUDU-1495. Maintenance manager should not schedule new ops during unregister
This fixes a bug where deleting a tablet could block for an extended
time on compactions being scheduled if the maintenance manager was
configured with multiple threads.
The issue was that Unregister() waited for the number of running
operations to reach 0 before removing the op from the candidate list of
ops, but did not prevent new instances of that op from being scheduled.
Change-Id: I3675705caf5b73f8a480036b974e4db6c205616a
---
M src/kudu/tablet/tablet.cc
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
4 files changed, 74 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/4295/2
--
To view, visit http://gerrit.cloudera.org:8080/4295
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3675705caf5b73f8a480036b974e4db6c205616a
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1495. Maintenance manager should not schedule new ops during unregister
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1495. Maintenance manager should not schedule new ops during unregister
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4295/1/src/kudu/util/maintenance_manager-test.cc
File src/kudu/util/maintenance_manager-test.cc:
Line 197: TEST_F(MaintenanceManagerTest, TestNewOpsDontGetScheduledDuringUnregister) {
> This test failed without the fix, right?
yessir
Line 209: });
> Nit: indentation.
hrm, really? this is how my emacs indents it, for better or worse... you think the } should line up with the 'A' above as if it were an if{} or while{}? I can try to convince emacs of that but this is more consistent with how lambdas are indented elsewhere.
http://gerrit.cloudera.org:8080/#/c/4295/1/src/kudu/util/maintenance_manager.h
File src/kudu/util/maintenance_manager.h:
Line 190: void Cancel() {
> Hmm. Cancel makes it sound like we're canceling a discrete run of the opera
was hoping that we can add some polling from within the ops themselves at some point so that they would actually abort themselves (eg if you delete a tablet mid-compaction it can just stop that compaction instead of completing it). Hence the above doc alluding to optionally polling.
--
To view, visit http://gerrit.cloudera.org:8080/4295
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675705caf5b73f8a480036b974e4db6c205616a
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-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1495. Maintenance manager should not schedule new ops during unregister
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1495. Maintenance manager should not schedule new ops during unregister
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4295
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675705caf5b73f8a480036b974e4db6c205616a
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1495. Maintenance manager should not schedule new ops during unregister
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1495. Maintenance manager should not schedule new ops during unregister
......................................................................
Patch Set 2:
Build Started http://104.196.14.100/job/kudu-gerrit/3200/
--
To view, visit http://gerrit.cloudera.org:8080/4295
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675705caf5b73f8a480036b974e4db6c205616a
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No