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 <din...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to