Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9551 )
Change subject: KUDU-2289: Tablet deletion should be throttled ...................................................................... Patch Set 3: (5 comments) Could you add a test for this? I'm confident that it hasn't regressed any functionality, but it'd be good for there to be a test that the throttling actually works as you'd expect. http://gerrit.cloudera.org:8080/#/c/9551/3/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/9551/3/src/kudu/tserver/tablet_service.cc@807 PS3, Line 807: context, req, resp > I don't understand how it's guaranteed that these parameters are not yet fr I think they're guaranteed to live until the call to context->RespondSuccess(). http://gerrit.cloudera.org:8080/#/c/9551/3/src/kudu/tserver/ts_tablet_manager.h File src/kudu/tserver/ts_tablet_manager.h: http://gerrit.cloudera.org:8080/#/c/9551/3/src/kudu/tserver/ts_tablet_manager.h@142 PS3, Line 142: Status DeleteTablet(const std::string& tablet_id, Could we make the synchronous version private? AFAICT it's now only used in an itest; perhaps it can use a Synchronizer to produce a cb that can be waited on? http://gerrit.cloudera.org:8080/#/c/9551/3/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/9551/3/src/kudu/tserver/ts_tablet_manager.cc@278 PS3, Line 278: virtual void DisableCallback() { This doesn't need to be virtual, does it? http://gerrit.cloudera.org:8080/#/c/9551/3/src/kudu/tserver/ts_tablet_manager.cc@840 PS3, Line 840: new DeleteTabletRunnable(this, tablet_id, delete_type, cas_config_index, cb)); Nit: use std::make_shared to make the runnable. http://gerrit.cloudera.org:8080/#/c/9551/3/src/kudu/tserver/ts_tablet_manager.cc@848 PS3, Line 848: if (s.IsServiceUnavailable()) { : cb(s, TabletServerErrorPB::THROTTLED); : return; : } : cb(s, TabletServerErrorPB::UNKNOWN_ERROR); Simpler as: cb(s, s.IsServiceUnavailable() ? THROTTLED : UNKNOWN_ERROR); -- To view, visit http://gerrit.cloudera.org:8080/9551 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3819bf8a3acf8ea03a76cc6cacd92d85bb114998 Gerrit-Change-Number: 9551 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Fri, 16 Mar 2018 23:26:48 +0000 Gerrit-HasComments: Yes
