You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dinesh Bhat (Code Review)" <ge...@cloudera.org> on 2016/11/23 00:50:04 UTC
[kudu-CR] [tools] Tombstone the tablet with "local replica delete"
Hello Mike Percy, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5191
to review the following change.
Change subject: [tools] Tombstone the tablet with "local_replica delete"
......................................................................
[tools] Tombstone the tablet with "local_replica delete"
This change makes the default action of 'local_replica delete'
tool to tombstone the tablet. To fully delete the tablet from
the node, user can use --clean_unsafe option.
Change-Id: Ia773de475431eb85fb0dbe724d524e8dd59b1b12
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
2 files changed, 156 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/5191/1
--
To view, visit http://gerrit.cloudera.org:8080/5191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia773de475431eb85fb0dbe724d524e8dd59b1b12
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [tools] Tombstone the tablet with "local replica delete"
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
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 <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools] Tombstone the tablet with "local replica delete"
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5191
to look at the new patch set (#2).
Change subject: [tools] Tombstone the tablet with "local_replica delete"
......................................................................
[tools] Tombstone the tablet with "local_replica delete"
This change makes the default action of 'local_replica delete'
tool to tombstone the tablet. To fully delete the tablet from
the node, user can use --clean_unsafe option.
Change-Id: Ia773de475431eb85fb0dbe724d524e8dd59b1b12
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
2 files changed, 146 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/5191/2
--
To view, visit http://gerrit.cloudera.org:8080/5191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia773de475431eb85fb0dbe724d524e8dd59b1b12
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [tools] Tombstone the tablet with "local replica delete"
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: [tools] Tombstone the tablet with "local_replica delete"
......................................................................
Patch Set 1:
(1 comment)
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
I think it makes sense to change defaults to be safer when possible. The primary downside in my opinion is that people used to the new version might do the wrong thing on the old version without realizing it but it's worth the risk in order to make safer operations the norm.
--
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 <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools] Tombstone the tablet with "local replica delete"
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged.
Change subject: [tools] Tombstone the tablet with "local_replica delete"
......................................................................
[tools] Tombstone the tablet with "local_replica delete"
This change makes the default action of 'local_replica delete'
tool to tombstone the tablet. Release 1.1 shipped the tool
with "--clean_unsafe" flag which let you delete the replica
from the disks. Deleting removes consensus metadata for the given
tablet and hence voilates Raft vote durability requirements.
Tombstoning on the other hand will preserve Raft votes for the
specfied tablet on the local node, hence is a safer choice
to be the default action for this tool.
This adds the support for tombstone action for this tool
which was unsupported in previous release.
Change-Id: Ia773de475431eb85fb0dbe724d524e8dd59b1b12
Reviewed-on: http://gerrit.cloudera.org:8080/5191
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
2 files changed, 145 insertions(+), 54 deletions(-)
Approvals:
Mike Percy: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia773de475431eb85fb0dbe724d524e8dd59b1b12
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [tools] Tombstone the tablet with "local replica delete"
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5191
to look at the new patch set (#4).
Change subject: [tools] Tombstone the tablet with "local_replica delete"
......................................................................
[tools] Tombstone the tablet with "local_replica delete"
This change makes the default action of 'local_replica delete'
tool to tombstone the tablet. Release 1.1 shipped the tool
with "--clean_unsafe" flag which let you delete the replica
from the disks. Deleting removes consensus metadata for the given
tablet and hence voilates Raft vote durability requirements.
Tombstoning on the other hand will preserve Raft votes for the
specfied tablet on the local node, hence is a safer choice
to be the default action for this tool.
This adds the support for tombstone action for this tool
which was unsupported in previous release.
Change-Id: Ia773de475431eb85fb0dbe724d524e8dd59b1b12
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
2 files changed, 144 insertions(+), 54 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/5191/4
--
To view, visit http://gerrit.cloudera.org:8080/5191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia773de475431eb85fb0dbe724d524e8dd59b1b12
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [tools] Tombstone the tablet with "local replica delete"
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: [tools] Tombstone the tablet with "local_replica delete"
......................................................................
Patch Set 5:
(2 comments)
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: ")
> yeah, unless you feel strongly about it, I decided not to special case Stat
SGTM.
http://gerrit.cloudera.org:8080/#/c/5191/4/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:
Line 789: .Description("Delete a Kudu replica from the local filesystem. "
> Done, to be consistent with other help strings, I haven't used 'tablet repl
Sorry about this, but I changed the terminology to "tablet replica" in https://gerrit.cloudera.org/5310 which is already merged. So let's use "tablet replica". You will also need a manual rebase.
--
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: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools] Tombstone the tablet with "local replica delete"
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools] Tombstone the tablet with "local replica delete"
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5191
to look at the new patch set (#3).
Change subject: [tools] Tombstone the tablet with "local_replica delete"
......................................................................
[tools] Tombstone the tablet with "local_replica delete"
This change makes the default action of 'local_replica delete'
tool to tombstone the tablet. Release 1.1 shipped the tool
with "--clean_unsafe" flag which let you delete the replica
from the disks. Deleting removes consensus metadata for the given
tablet and hence voilates Raft vote durability requirements.
Tombstoning on the other hand will preserve Raft votes for the
specfied tablet on the local node, hence is a safer choice
to be the default action for this tool.
This adds the support for tombstone action for this tool
which was unsupported in previous release.
Change-Id: Ia773de475431eb85fb0dbe724d524e8dd59b1b12
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
2 files changed, 148 insertions(+), 54 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/5191/3
--
To view, visit http://gerrit.cloudera.org:8080/5191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia773de475431eb85fb0dbe724d524e8dd59b1b12
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [tools] Tombstone the tablet with "local replica delete"
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: [tools] Tombstone the tablet with "local_replica delete"
......................................................................
Patch Set 4:
(2 comments)
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: ")
> The way to appease tidy-bot is like this:
yeah, unless you feel strongly about it, I decided not to special case Status::Corruption upon second thoughts. Current prefix of 'Error in log segment' should cover any present/future error returned from ReadNextEntry().
http://gerrit.cloudera.org:8080/#/c/5191/4/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:
Line 789: .Description("Tombstone the Kudu replica in the local filesystem")
> How about: "Delete a tablet replica from the local system. By default, leav
Done, to be consistent with other help strings, I haven't used 'tablet replica', but sticking to 'Kudu replica'.
--
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools] Tombstone the tablet with "local replica delete"
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat 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,
> 0) Yeah I saw that after reading this part, so it's certainly not crazy, bu
Thanks Mike for explanations, I modified the paramter as per above discussion.
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
Cool, tx.
Line 185: s.CloneAndPrepend("Corruption in log segment: ");
> This is a no-op... s is not mutated, it is cloned and returned.
That would upset tidy-bot [readability-issue-with-else-after-return] or something like that. I think that was the partial reason why I had s = Status:: earlier there. I think RETURN_NOT_OK_PREPEND is a good choice, updated.
Line 308: TabletDataState state;
> How about default state = TabletDataState::TABLET_DATA_DELETED and skip the
Done
Line 314: if (s.ok() || s.IsNotFound()) {
> How about:
Nice, took that verbatim.
PS3, Line 315: state = TabletDataState::TABLET_DATA_TOMBSTONED
> This should always be set as part of if (!FLAGS_clean_unsafe), you can move
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools] Tombstone the tablet with "local replica delete"
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: [tools] Tombstone the tablet with "local_replica delete"
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5191/4/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:
Line 789: .Description("Tombstone the Kudu replica in the local filesystem")
How about: "Delete a tablet replica from the local system. By default, leaves a tombstone record."
--
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools] Tombstone the tablet with "local replica delete"
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5191
to look at the new patch set (#6).
Change subject: [tools] Tombstone the tablet with "local_replica delete"
......................................................................
[tools] Tombstone the tablet with "local_replica delete"
This change makes the default action of 'local_replica delete'
tool to tombstone the tablet. Release 1.1 shipped the tool
with "--clean_unsafe" flag which let you delete the replica
from the disks. Deleting removes consensus metadata for the given
tablet and hence voilates Raft vote durability requirements.
Tombstoning on the other hand will preserve Raft votes for the
specfied tablet on the local node, hence is a safer choice
to be the default action for this tool.
This adds the support for tombstone action for this tool
which was unsupported in previous release.
Change-Id: Ia773de475431eb85fb0dbe724d524e8dd59b1b12
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
2 files changed, 145 insertions(+), 54 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/5191/6
--
To view, visit http://gerrit.cloudera.org:8080/5191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia773de475431eb85fb0dbe724d524e8dd59b1b12
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [tools] Tombstone the tablet with "local replica delete"
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5191
to look at the new patch set (#5).
Change subject: [tools] Tombstone the tablet with "local_replica delete"
......................................................................
[tools] Tombstone the tablet with "local_replica delete"
This change makes the default action of 'local_replica delete'
tool to tombstone the tablet. Release 1.1 shipped the tool
with "--clean_unsafe" flag which let you delete the replica
from the disks. Deleting removes consensus metadata for the given
tablet and hence voilates Raft vote durability requirements.
Tombstoning on the other hand will preserve Raft votes for the
specfied tablet on the local node, hence is a safer choice
to be the default action for this tool.
This adds the support for tombstone action for this tool
which was unsupported in previous release.
Change-Id: Ia773de475431eb85fb0dbe724d524e8dd59b1b12
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
2 files changed, 145 insertions(+), 54 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/5191/5
--
To view, visit http://gerrit.cloudera.org:8080/5191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia773de475431eb85fb0dbe724d524e8dd59b1b12
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [tools] Tombstone the tablet with "local replica delete"
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: [tools] Tombstone the tablet with "local_replica delete"
......................................................................
Patch Set 6: Code-Review+2
--
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: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [tools] Tombstone the tablet with "local replica delete"
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [tools] Tombstone the tablet with "local_replica delete"
......................................................................
Patch Set 2:
(2 comments)
Looks good to me, modulo two small things. I'll defer to Mike for the +2.
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.
> Adar, fyi what we shipped in 1.1 was a tool allowing the deletion via '--cl
I see. Can you reword this paragraph then, to make the (lack of a semantic) change more clear?
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:
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_NOT_OK() on L185?
--
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 <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools] Tombstone the tablet with "local replica delete"
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [tools] Tombstone the tablet with "local_replica delete"
......................................................................
Patch Set 1:
(6 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 delete' shipped in 1.1?
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 empty line helps separate and emphasize the comment.
Line 1156: // Grab the tablet_id to delete
Nit: terminate with a period.
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 "external" (by sending RPCs to it like this) and "internal" (by making calls directly into in-memory data structures e.g. Tablet::Flush() or TsTabletManager::LookupTablet()).
In this case, you could use TsTabletManager::GetTabletPeers to find the ID of the single tablet, right? So can you use that here and below, to avoid doing any RPCs from the test whatsoever?
Can you also make the same change to the test that much of this was copied from?
Line 1185: if (tablet_peer->log()) {
Why isn't this guaranteed to be true? As written, it's unclear when the test will run and verify the tombstoned_opid with last_logged_opid, and when it won't. Can we change the test setup to guarantee the presence of last_logged_opid?
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:
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 last thing on the previous line.
--
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 <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools] Tombstone the tablet with "local replica delete"
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
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 <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools] Tombstone the tablet with "local replica delete"
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: [tools] Tombstone the tablet with "local_replica delete"
......................................................................
Patch Set 3:
(1 comment)
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: :
> Cool, tx.
The way to appease tidy-bot is like this:
if (s.IsCorruption()) {
return s.CloneAndPrepend("Corruption in log segment");
}
if (s.IsEndOfFile()) {
break;
}
RETURN_NOT_OK(s);
--
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 <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools] Tombstone the tablet with "local replica delete"
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools] Tombstone the tablet with "local replica delete"
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: [tools] Tombstone the tablet with "local_replica delete"
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5191/4/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:
Line 789: .Description("Tombstone the Kudu replica in the local filesystem")
> Sorry about this, but I changed the terminology to "tablet replica" in http
Done, didn't see those changes flying by. I must have been asleep. Manually rebased now.
--
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes