Todd Lipcon has posted comments on this change. Change subject: KUDU-921. tablet copy: Make the StartTabletCopy() RPC async ......................................................................
Patch Set 12: (5 comments) http://gerrit.cloudera.org:8080/#/c/5045/12/src/kudu/integration-tests/tablet_copy_client_session-itest.cc File src/kudu/integration-tests/tablet_copy_client_session-itest.cc: PS12, Line 145: kBlank, : kTombstoned, : k a description of these two scenarios would be useful. In paricular I'm not really sure what the 'blank' scenario means. PS12, Line 150: // Initialize state. : TabletDataState delete_type = TabletDataState::TABLET_DATA_UNKNOWN; : switch (scenario) { : case kBlank: : break; : case kTombstoned: : delete_type = TabletDataState::TABLET_DATA_TOMBSTONED; : break; : default: : LOG(FATAL) << "Unsupported Scenario"; : } : if (delete_type != TabletDataState::TABLET_DATA_UNKNOWN) { : NO_FATALS(DeleteTabletWithRetries(ts1, tablet_id, delete_type, boost::none, kTimeout)); : } instead of all of this, couldn't you just do: if (scenario == kTombstoned) { NO_FATALS(DeleteTabletWithRetries(... TABLET_DATA_TOMBSTONED); } http://gerrit.cloudera.org:8080/#/c/5045/12/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: Line 323: Status _s = (expr); /* NOLINT */ \ why not do 'const Status& _s' here? then lint wouldn't complain, right? Line 373: virtual void Cancel() { hrm, is this overriding a Runnable method? surprised to see it virtual but not override. Maybe naming this something like DontCallCallback would be more clear that it's not something inherent to the Runnable interface Line 820: // TODO(mpercy): Cancel all outstanding tablet copy tasks. hrm, is this an important todo? sounds important, so maybe there should be a JIRA? -- To view, visit http://gerrit.cloudera.org:8080/5045 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I95c63f2bfd67624844447862efbdba9cb3676112 Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
