Mike Percy has posted comments on this change. Change subject: [tools] Tombstone the tablet with "local_replica delete" ......................................................................
Patch Set 2: (11 comments) overall looks good, mostly minor comments http://gerrit.cloudera.org:8080/#/c/5191/2/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: Line 1072: ASSERT_EQ(tablet_peers.size(), 1); nit: expected value comes first in the ASSERT_EQ macro Line 1117: { nit: These extra braces seem superfluous here. Line 1120: ASSERT_EQ(tablet_peers.size(), 0); nit: reversed order of ASSERT_EQ arguments Line 1154: ASSERT_EQ(tablet_peers.size(), 1); reverse args Line 1156: ASSERT_TRUE(tablet_peers[0]->log() != nullptr); remove this line? seems over-defensive Line 1158: ASSERT_TRUE(last_logged_opid.IsInitialized()); why this line? Maybe you mean ASSERT_FALSE(OpIdEquals(last_logged_opid, MinimumOpId())); Line 1190: ASSERT_EQ(tablet_peers[0]->tablet_metadata()->tablet_data_state(), nit: order of ASSERT_EQ args here and below 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 165: Status FindLastCommittedOpId(FsManager* fs, const string& tablet_id, doc Line 166: boost::optional<OpId>* last_committed_opid) { I don't think boost::optional<OpId>* should be used as an out-parameter here. Just use an OpId*. optional<> gives no additional information because the semantics are that if the function returns OK then we found the OpId, otherwise we didn't. I think optional<> is useful in c++ without exceptions in the following cases: - optional input parameters - when outputting many values from a function and some may or may not be set PS2, Line 180: s = Status::Corruption( : Substitute("Corruption detected in log segment: $0", s.ToString())); > Why do we rewrite 's' in this case? Why not return it as-is during RETURN_N I agree with Adar unless the existing error message isn't understandable. If we really want to prepend a message, this would be preferred: return s.CloneAndPrepend("Corruption detected in log segment") or s = s.CloneAndPrepend("Corruption detected in log segment") Line 189: if (last_committed_opid->is_initialized()) return Status::OK(); Just use a separate bool for this -- 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: 2 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
