Mike Percy has posted comments on this change. Change subject: KUDU-2114. Don't re-delete tombstoned replicas ......................................................................
Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/7842/1/src/kudu/integration-tests/delete_table-itest.cc File src/kudu/integration-tests/delete_table-itest.cc: Line 1170: // The table should have replicas on all three tservers. > "all" three? But there are 4 tservers. Done Line 1249: // Ensure that number of delete attempts does not climb for the next several seconds. > Rather than waiting for a number of seconds to allow heartbeats to flow, pe Done http://gerrit.cloudera.org:8080/#/c/7842/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: > I'd rebase on the top of the tree; you'll have to resolve some LOG/VLOG con Done Line 2496: if ((tablet_state == tablet::RUNNING || tablet_state == tablet::FAILED) && > This effectively keeps out any tablets in INITIALIZED, BOOTSTRAPPING, STOPP I was trying to keep this as similar to the previous logic as possible, which only ran while the tablet was "online". But I don't think the change was necessary, so I'll revert this. Line 2510: TabletDataState data_state = report.tablet_data_state(); > Used in just one place; inline it? Done Line 2512: data_state == TABLET_DATA_READY && > Why enforce TABLET_DATA_READY instead of enforcing "not TABLET_DATA_DELETED I was trying to be conservative, but I think your suggestion is more intuitive and still safe. We won't currently allow deletion of a copying tablet but we may in the future. Line 2526: if (tablet_state == tablet::RUNNING && > Not getting this one either; does this imply that the reported cstate of a Same as above, however I suppose this change should not be necessary because the cstate should be reliable enough, considering committed config opid is checked. So I'll revert this change. Line 2537: if (tablet_state == tablet::RUNNING && > Likewise; we transition to running because of the leader info present in th Ah, this check is already performed in ShouldTransitionTabletToRunning(), so it's superfluous. Removing this too. -- To view, visit http://gerrit.cloudera.org:8080/7842 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I86e5cbe0681557ae86f5bae53f4aeb7635fa6aa6 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-HasComments: Yes
