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 <t...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to