Dinesh Bhat has posted comments on this change. Change subject: [tools] Tombstone the tablet with "local_replica delete" ......................................................................
Patch Set 2: (12 comments) TFTR Adar/Mike, addressed comments, one response inline below: 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. > I see. Can you reword this paragraph then, to make the (lack of a semantic) Done 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 Done Line 1117: { > nit: These extra braces seem superfluous here. Done Line 1120: ASSERT_EQ(tablet_peers.size(), 0); > nit: reversed order of ASSERT_EQ arguments Done Line 1154: ASSERT_EQ(tablet_peers.size(), 1); > reverse args Done Line 1156: ASSERT_TRUE(tablet_peers[0]->log() != nullptr); > remove this line? seems over-defensive Done Line 1158: ASSERT_TRUE(last_logged_opid.IsInitialized()); > why this line? Maybe you mean Naah, being extra defensive here too, it's not equal to MinimumOpID btw because we have generated some workload so OpId index is set to some sane integer value with last_logged_opid. Line 1190: ASSERT_EQ(tablet_peers[0]->tablet_metadata()->tablet_data_state(), > nit: order of ASSERT_EQ args here and below Done 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 Done Line 166: boost::optional<OpId>* last_committed_opid) { > I don't think boost::optional<OpId>* should be used as an out-parameter her Hmmm, couple of things I want to mention before I take your suggestion: 0) DeleteTabletData() at L327 takes an optional<OpId>, and that was the original motive to use optional here. 1) This routine could return NotFound as well which we treat as success in the caller and proceed with Tombstoning. 2) This may fall under the second use case for 'optional' you mentioned above, no ? i.e in a situation where output may be set or unset. 3) If we don't want to keep 'optional' out param, we could either copy the OpId returned here in the caller or have a if/else combined with boost::none while calling DeleteTabletData(). So, I am kinda still favoring the current approach we have. Let me know. PS2, Line 180: s = Status::Corruption( : Substitute("Corruption detected in log segment: $0", s.ToString())); > I agree with Adar unless the existing error message isn't understandable. I Done, yeah I wanted to be bit more verbose than the error string returned by ReadNextEntry(). Line 189: if (last_committed_opid->is_initialized()) return Status::OK(); > Just use a separate bool for this 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: 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
