Mike Percy has posted comments on this change. Change subject: [tools] Tombstone the tablet with "local_replica delete" ......................................................................
Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/5191/2/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: Line 166: Status FindLastCommittedOpId(FsManager* fs, const string& tablet_id, > Hmmm, couple of things I want to mention before I take your suggestion: 0) Yeah I saw that after reading this part, so it's certainly not crazy, but I still think it's better to keep a clear contract on a per-method basis. 1) True, but in all non-OK cases, OpId will not be set. So we have sufficient information without the optional<> 2) I don't think so. There is only one output, plus an optional error message. If you don't get an error, the output should be ignored. 3) We already have to have per-error logic in the caller, so it should not be much of a burden. I've added some comments over there about this. http://gerrit.cloudera.org:8080/#/c/5191/3/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: PS3, Line 185: : You don't need the ": " part, that is automatically handled for you. If you add it, you will get "Corruption in log segment: : <rest of the message>" Line 185: s.CloneAndPrepend("Corruption in log segment: "); This is a no-op... s is not mutated, it is cloned and returned. How about just: return s.CloneAndPrepend("Corruption in log segment"); Line 308: TabletDataState state; How about default state = TabletDataState::TABLET_DATA_DELETED and skip the else block below Line 314: if (s.ok() || s.IsNotFound()) { How about: OpId opid; Status s = FindLastCommittedOpId(&fs_manager, tablet_id, &opid); if (s.ok()) { last_committed_opid = opid; } else if (s.IsNotFound()) { LOG(INFO) << "Could not find last committed OpId from WAL, but proceeding with tablet tombstone: " << s.ToString(); } else { LOG(ERROR) << "Error attempting to find last committed OpId in WAL: " << s.ToString(); LOG(ERROR) << "Cannot delete (tombstone) the tablet, use --clean_unsafe to delete" << " the tablet permanently from this server."; return s; } PS3, Line 315: state = TabletDataState::TABLET_DATA_TOMBSTONED This should always be set as part of if (!FLAGS_clean_unsafe), you can move it up before FindLastCommittedOpId() -- 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: 3 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
