Adar Dembo 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. 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, perhaps you can condition on TSHeartbeat RPC metrics? Then you can stop after some fixed number of heartbeats, potentially speeding up the test too. 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 conflicts. Line 2496: if ((tablet_state == tablet::RUNNING || tablet_state == tablet::FAILED) && This effectively keeps out any tablets in INITIALIZED, BOOTSTRAPPING, STOPPING, STOPPED, and SHUTDOWN. I understand keeping out STOPPING since it's such a short term transient state, but why the others? For example, why shouldn't we ask a shut down tablet to delete itself if it's been evicted? Line 2510: TabletDataState data_state = report.tablet_data_state(); Used in just one place; inline it? Line 2512: data_state == TABLET_DATA_READY && Why enforce TABLET_DATA_READY instead of enforcing "not TABLET_DATA_DELETED and not TABLET_DATA_TOMBSTONED"? Isn't it legal to delete COPYING tablets if they've been evicted? Line 2526: if (tablet_state == tablet::RUNNING && Not getting this one either; does this imply that the reported cstate of a FAILED tablet cannot be trusted? Line 2537: if (tablet_state == tablet::RUNNING && Likewise; we transition to running because of the leader info present in the reported cstate. Should we distrust a FAILED tablet's cstate? -- 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-HasComments: Yes
