Dinesh Bhat has posted comments on this change. Change subject: [tools] Tombstone the tablet with "local_replica delete" ......................................................................
Patch Set 1: (9 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 dele Adar, fyi what we shipped in 1.1 was a tool allowing the deletion via '--clean_unsafe' flag. i.e If I had used the tool without the flag, the 1.1 threw an unsupported error. Now we are adding the tombstone support post 1.1 while retaining old behavior for clean_unsafe. So the semantic really didn't change as such. 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 emp agreed, and I prefer that too. This wasn't intentional removal, thanks for catching it. fixed. Line 1156: // Grab the tablet_id to delete > Nit: terminate with a period. Done 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 "ext Yeah good points, fixed at both places. Line 1185: if (tablet_peer->log()) { > Why isn't this guaranteed to be true? As written, it's unclear when the tes yeah, 'if (tablet_peer->log())' check here was moot and not needed. What was not clear to me at the time was whether we have any other situation I am unaware of where last_entry_op_id_ may not be initialized for a tablet. So wanted to be on the safer side. I think that situation doesn't exist for 2 reasons: 1) We are generating some workload and waiting for them to finish. So we expect the last_entry_op_id_ to be initialized at this stage. 2) Between this and L1231 the last_logged_opid isn't expected to change, so we can remove the check at L1230 as well. I also added asserts at both places to check opids are initialized. 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: Line 97: using consensus::ReplicateMsg; > warning: using decl 'ReplicateMsg' is unused [misc-unused-using-decls] Done Line 167: boost::optional<OpId>& last_committed_opid) { > warning: non-const reference parameter 'last_committed_opid', make it const Done Line 183: } else if (s.IsEndOfFile()) { > warning: don't use else after return [readability-else-after-return] Done 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 Done -- 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: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
