Adar Dembo has posted comments on this change. Change subject: [tools] Tombstone the tablet with "local_replica delete" ......................................................................
Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/5191/1//COMMIT_MSG Commit Message: PS1, Line 9: This change makes the default action of 'local_replica delete' : tool to tombstone the tablet. Are we comfortable with this semantic change given that 'local_replica delete' shipped in 1.1? http://gerrit.cloudera.org:8080/#/c/5191/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: Line 1089 Nit: why remove this? Stylistically I think preceding a comment with an empty line helps separate and emphasize the comment. Line 1156: // Grab the tablet_id to delete Nit: terminate with a period. PS1, Line 1157: ListTabletsRequestPB req; : ListTabletsResponsePB resp; : RpcController rpc; : rpc.set_timeout(kTimeout); : { : unique_ptr<TabletServerServiceProxy> ts_proxy; : ASSERT_OK(BuildProxy(ts->bound_rpc_addr().ToString(), : tserver::TabletServer::kDefaultPort, &ts_proxy)); : ASSERT_OK(ts_proxy->ListTablets(req, &resp, &rpc)); : } : ASSERT_FALSE(resp.has_error()); : ASSERT_EQ(resp.status_and_schema_size(), 1); : const string& tablet_id = resp.status_and_schema(0).tablet_status().tablet_id(); It's a little weird to see a test that treats the mini cluster as both "external" (by sending RPCs to it like this) and "internal" (by making calls directly into in-memory data structures e.g. Tablet::Flush() or TsTabletManager::LookupTablet()). In this case, you could use TsTabletManager::GetTabletPeers to find the ID of the single tablet, right? So can you use that here and below, to avoid doing any RPCs from the test whatsoever? Can you also make the same change to the test that much of this was copied from? Line 1185: if (tablet_peer->log()) { Why isn't this guaranteed to be true? As written, it's unclear when the test will run and verify the tombstoned_opid with last_logged_opid, and when it won't. Can we change the test setup to guarantee the presence of last_logged_opid? http://gerrit.cloudera.org:8080/#/c/5191/1/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: PS1, Line 315: << "Can not delete (tombstone) the tablet, use --clean_unsafe to delete" : << " the tablet permanently from the node."; Nit: use a new LOG(WARNING) for this part, to allow s.ToString() to be the last thing on the previous line. -- To view, visit http://gerrit.cloudera.org:8080/5191 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia773de475431eb85fb0dbe724d524e8dd59b1b12 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
