You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Will Berkeley (Code Review)" <ge...@cloudera.org> on 2017/05/01 17:39:21 UTC

[kudu-CR] [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Will Berkeley has uploaded a new change for review.

http://gerrit.cloudera.org:8080/6772

Change subject: [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................

[WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

This patch enhances ksck to gather consensus info from every tablet. It
compares this info with master and outputs the master's config and every
conflicting config, if there are any conflicts. This checking is expensive
because it requires gathering consensus info from every replica, so
it is off by default.

This will catch at least the two problems identified in KUDU-1860:
1. The leader has a pending config to remove a tablet, but it is not
committed so the master does not see this config. This can hide an
unhealthy tablet if, e.g., one pending config member is down and the
pending-to-be-kicked-out member is up, so 1/2 replicas are alive in
the leader's active config but the master thinks 2/3 are alive.
2. No replica is leader but the master believes there is a leader
because its cache is old and hasn't been updated.

Sample output:
1. https://gist.github.com/wdberkeley/aa97e9a108b57acacc4da6db0625445b
2. (problem #1) https://gist.github.com/wdberkeley/b8f9987caf8bd15ec4fd8fd345a217f9

WIP because it needs tests.

Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
5 files changed, 178 insertions(+), 8 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/6772/1
--
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>

[kudu-CR] [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.

Will Berkeley has posted comments on this change.

Change subject: [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................


Patch Set 1:

(8 comments)

I implemented a GetAllConsensusStates RPC and used it instead of one GetConsensusState RPC per tablet replica; however, this makes the new features available only for servers >= 1.4. Should we stick both forms of logic in so it works on older servers?

http://gerrit.cloudera.org:8080/#/c/6772/1/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

PS1, Line 210: s.ToString()
> This s refers to the one from line 204, when I assume it should be the one 
Done


PS1, Line 616: shared_ptr<KsckTabletReplica> r
> const shared_ptr<KsckTabletReplica>& r
Done


PS1, Line 624: std::shared_ptr<KsckTabletReplica> r
> same
Done


Line 716:   } else if (consensus_conflict) {
> If we catch a replica in the middle of a config change then we can still se
Yes, that's part of a core problem with these consensus checks: we can't retrieve the information from all servers simultaneously, and config changes can happen as a normal part of the tablet's operation. However, AFAICT consensus doesn't track any timestamps on its configs, so we'd have to introduce that mechanism just for these checks. Would it be better to, instead of surfacing all conflicts, have some known bad situations that we check for and alert on? We could additionally allow ALL info to be printed, so a knowledgable admin could use that to investigate misbehaving tablets that our heuristics don't cover.


http://gerrit.cloudera.org:8080/#/c/6772/1/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:

PS1, Line 100: peers
> you should also std::move(peers) here
Tidy bot caught that on patch set 2; it's done in set 3.


PS1, Line 124:   int64_t term_;
             :   boost::optional<string> leader_uuid_;
             :   vector<string> peers_;
> it looks like these can all be const
If you make them const it breaks the assignment at ksck.cc L663 since the copy-assign operator is implicitly deleted. I think the compiler with copy-elide there.


http://gerrit.cloudera.org:8080/#/c/6772/1/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

PS1, Line 114: for (auto it = tablet_status_map_.begin(); it != tablet_status_map_.end(); it++)
> Since you're not mutating tablet_status_map_, it's more idiomatic to write 
Done


PS1, Line 120: InsertOrDi
> It's not safe to trust that multiple remotes won't have the same UUID and t
Good point.


-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................


Patch Set 10: Code-Review+1

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:

Line 453:       "  config from master:  A*  B   C                                 Yes       \n"
> Hm you'd have to actually get the RPC to return duplicates from 1 or more t
One way to test this is to put replicas on multiple TSes, shut the TSes down, replace one of the TS UUIDs, and restart the cluster. I see your point about the state of the test framework for KsckTest


http://gerrit.cloudera.org:8080/#/c/6772/9/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:

Line 396: TEST_F(KsckTest, TestConsensusConflictExtraPeer) {
> This isn't an integration test. I went to go find the tool-itest I thought 
Oh, yeah. This test is a bit yuck.


http://gerrit.cloudera.org:8080/#/c/6772/10/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:

PS10, Line 119:  are match
extra word: are


-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/6772

to look at the new patch set (#4).

Change subject: [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................

[WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

This patch enhances ksck to gather consensus info from every tablet. It
compares this info with master and outputs the master's config and every
conflicting config, if there are any conflicts. To do this efficiently
it implements a new GetAllConsensusStates RPC that gathers info about
every replica's consensus state.

Sample output:
1. https://gist.github.com/wdberkeley/aa97e9a108b57acacc4da6db0625445b
2. (problem #1) https://gist.github.com/wdberkeley/b8f9987caf8bd15ec4fd8fd345a217f9

WIP because it needs tests.

Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
---
M src/kudu/consensus/consensus.proto
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
8 files changed, 255 insertions(+), 18 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/6772/4
--
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................


Patch Set 1:

(8 comments)

I wonder if we should add an RPC that will return all { committed, pending } configs from all replicas hosted on that tablet server. If we can pull that from all servers simultaneously (using the Async RPC API) we could turn this on by default since it shouldn't be too slow.

http://gerrit.cloudera.org:8080/#/c/6772/1/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

PS1, Line 210: s.ToString()
This s refers to the one from line 204, when I assume it should be the one from line 208


PS1, Line 616: shared_ptr<KsckTabletReplica> r
const shared_ptr<KsckTabletReplica>& r


PS1, Line 624: std::shared_ptr<KsckTabletReplica> r
same


Line 716:   } else if (consensus_conflict) {
If we catch a replica in the middle of a config change then we can still see this, which makes this warning a little racy. I wonder if we couldn't do something to make sure it's "stuck" like add a metric or a flag to the output given from the servers to indicate how long a config change has been pending.


http://gerrit.cloudera.org:8080/#/c/6772/1/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:

PS1, Line 100: peers
you should also std::move(peers) here


PS1, Line 124:   int64_t term_;
             :   boost::optional<string> leader_uuid_;
             :   vector<string> peers_;
it looks like these can all be const


http://gerrit.cloudera.org:8080/#/c/6772/1/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

PS1, Line 114: for (auto it = tablet_status_map_.begin(); it != tablet_status_map_.end(); it++)
Since you're not mutating tablet_status_map_, it's more idiomatic to write this using C++11 for-loop syntax like this:

  for (const auto& entry : tablet_status_map_) {
    req.set_tablet_id(entry.first);
    // ...


PS1, Line 120: InsertOrDi
It's not safe to trust that multiple remotes won't have the same UUID and tablet id in cases where people did weird stuff with restoring backups. It's bad, and it indicates a misconfiguration, but it shouldn't crash the client tool.


-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6772

to look at the new patch set (#7).

Change subject: KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................

KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

This patch enhances ksck to gather consensus info from every
tablet. It compares this info with master and outputs the
master's config and every conflicting config, if there are any
conflicts. To do this efficiently it implements a new
GetAllConsensusStates RPC that gathers info about every
replica's consensus state.

This will catch at least the two problems identified in
KUDU-1860: 1. The leader has a pending config to remove a
tablet, but it is not committed so the master does not see this
config. This can hide an unhealthy tablet if, e.g., one pending
config member is down and the pending-to-be-kicked-out member is
up, so 1/2 replicas are alive in the leader's active config but
the master thinks 2/3 are alive. 2. No replica is leader but the
master believes there is a leader because its cache is old and
hasn't been updated.

Sample output showing #1:
https://gist.github.com/wdberkeley/f964439bb0c407d3769c2dd9a0959ca0

Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
---
M src/kudu/consensus/consensus.proto
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
9 files changed, 411 insertions(+), 24 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/6772/7
-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.

Will Berkeley has posted comments on this change.

Change subject: KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................


Patch Set 7:

(8 comments)

Both of your comments about more testing are things that aren't done well by the current ksck tests. I think the takeaway is there should by a ksck- or tool-itest to cover these situations. It would also be useful as a place to see what situations ksck should detect.

http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:

Line 453: 
> Sounds good. I'm just wondering if we can make a test case for that scenari
Hm you'd have to actually get the RPC to return duplicates from 1 or more ts. Another reason to add a tool itest like I mentioned in another comment.


http://gerrit.cloudera.org:8080/#/c/6772/9/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:

Line 396: 
> Can we also add a test case with Committed == no?
This isn't an integration test. I went to go find the tool-itest I thought would exist and there isn't one. Looks like a todo: add a tool-itest or a ksck-itest.


http://gerrit.cloudera.org:8080/#/c/6772/9/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

PS9, Line 604:  string tabl
> nit: how about ReplicaInfo since we have an important consensus class calle
Done


Line 612:     boost::optional<tablet::TabletStatusPB> status_pb;
> nit: Your code comments should all end with punctuation per https://google.
Done


Line 672:                                                          repl_leader_uuid,
> not used? also missing ending ]
Done


http://gerrit.cloudera.org:8080/#/c/6772/9/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:

Line 123:   }
> When I see operator== I think that it's only going to return true for struc
Done


http://gerrit.cloudera.org:8080/#/c/6772/9/src/kudu/tserver/tablet_replica_lookup.h
File src/kudu/tserver/tablet_replica_lookup.h:

Line 54: 
> Add comment: Appends all non-tombstoned tablet replicas to the 'replicas' v
Done


http://gerrit.cloudera.org:8080/#/c/6772/9/src/kudu/tserver/tablet_service.h
File src/kudu/tserver/tablet_service.h:

Line 183:                                  consensus::GetConsensusStateResponsePB* resp,
> nit: Indentation should look like GetLastOpId()
Fixed all the off indents in this section.


-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.

Will Berkeley has posted comments on this change.

Change subject: KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6772/10/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:

PS10, Line 119:  are match
> extra word: are
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/6772/4/src/kudu/consensus/consensus.proto
File src/kudu/consensus/consensus.proto:

PS4, Line 471: optional
why not required?


Line 474:   repeated bytes tablet_ids = 2;
Maybe we should specify that if this is empty then it means fetch consensus info for all of the tablets


Line 480:     optional ConsensusStatePB cstate = 2;
how about:

  message GetAllConsensusStatesResponsePB {
    message TabletConsensusInfoPB {
      required bytes tablet_id = 1;
      optional int64 current_term = 2;
      optional bytes leader_uuid = 3;
      optional RaftConfigPB committed_config = 4;
      optional RaftConfigPB pending_config = 5;
    }
    repeated TabletConsensusInfoPB tablets = 1;
    optional tserver.TabletServerErrorPB error = 2;
  }

And then only fill in pending_config if there is a config change pending.


http://gerrit.cloudera.org:8080/#/c/6772/4/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:

PS4, Line 1074: CONSENSUS_CONFIG_ACTIVE
We may want to plumb in a way to get both the committed and pending consensus states at the same time.


-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.

Will Berkeley has posted comments on this change.

Change subject: [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/6772/1/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:

Line 69:       const std::string& tablet_id,
> warning: parameter 'tablet_id' is unused [misc-unused-parameters]
Expected.


Line 70:       const Schema& schema,
> warning: parameter 'schema' is unused [misc-unused-parameters]
Expected.


http://gerrit.cloudera.org:8080/#/c/6772/1/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

Line 630:   for (const shared_ptr<KsckTabletReplica> replica : tablet->replicas()) {
> warning: the loop variable's type is not a reference type; this creates a c
Done


Line 659:         if (consensus_pb.has_leader_uuid() && consensus_pb.leader_uuid().size() > 0) {
> warning: the 'empty' method should be used to check for emptiness instead o
Done


http://gerrit.cloudera.org:8080/#/c/6772/1/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:

Line 99:       leader_uuid_(leader_uuid),
> warning: value argument 'leader_uuid' can be moved to avoid copy [misc-move
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.

Hello Mike Percy, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6772

to look at the new patch set (#11).

Change subject: KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................

KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

This patch enhances ksck to gather consensus info from every
tablet. It compares this info with master and outputs the
master's config and every conflicting config, if there are any
conflicts. To do this efficiently it reimplements the
GetAllConsensusState RPC so that it gathers info about every
replica's consensus state.

This will catch at least the two problems identified in
KUDU-1860: 1. The leader has a pending config to remove a
tablet, but it is not committed so the master does not see this
config. This can hide an unhealthy tablet if, e.g., one pending
config member is down and the pending-to-be-kicked-out member is
up, so 1/2 replicas are alive in the leader's active config but
the master thinks 2/3 are alive. 2. No replica is leader but the
master believes there is a leader because its cache is old and
hasn't been updated.

Sample output showing #1:
https://gist.github.com/wdberkeley/d2606698e4f2e8ca3ef70d4dcef7ba9a

Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
---
M src/kudu/consensus/consensus.proto
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tserver/tablet_replica_lookup.h
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
M src/kudu/tserver/ts_tablet_manager.h
14 files changed, 521 insertions(+), 71 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/6772/11
-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6772

to look at the new patch set (#10).

Change subject: KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................

KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

This patch enhances ksck to gather consensus info from every
tablet. It compares this info with master and outputs the
master's config and every conflicting config, if there are any
conflicts. To do this efficiently it reimplements the
GetAllConsensusState RPC so that it gathers info about every
replica's consensus state.

This will catch at least the two problems identified in
KUDU-1860: 1. The leader has a pending config to remove a
tablet, but it is not committed so the master does not see this
config. This can hide an unhealthy tablet if, e.g., one pending
config member is down and the pending-to-be-kicked-out member is
up, so 1/2 replicas are alive in the leader's active config but
the master thinks 2/3 are alive. 2. No replica is leader but the
master believes there is a leader because its cache is old and
hasn't been updated.

Sample output showing #1:
https://gist.github.com/wdberkeley/d2606698e4f2e8ca3ef70d4dcef7ba9a

Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
---
M src/kudu/consensus/consensus.proto
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tserver/tablet_replica_lookup.h
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
M src/kudu/tserver/ts_tablet_manager.h
14 files changed, 521 insertions(+), 71 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/6772/10
-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................


Patch Set 7:

btw feel free to push back on stuff like naming nits if you disagree

-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

[kudu-CR] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6772

to look at the new patch set (#8).

Change subject: KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................

KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

This patch enhances ksck to gather consensus info from every
tablet. It compares this info with master and outputs the
master's config and every conflicting config, if there are any
conflicts. To do this efficiently it reimplements the
GetAllConsensusState RPC so that it gathers info about every
replica's consensus state.

This will catch at least the two problems identified in
KUDU-1860: 1. The leader has a pending config to remove a
tablet, but it is not committed so the master does not see this
config. This can hide an unhealthy tablet if, e.g., one pending
config member is down and the pending-to-be-kicked-out member is
up, so 1/2 replicas are alive in the leader's active config but
the master thinks 2/3 are alive. 2. No replica is leader but the
master believes there is a leader because its cache is old and
hasn't been updated.

Sample output showing #1:
https://gist.github.com/wdberkeley/d2606698e4f2e8ca3ef70d4dcef7ba9a

Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
---
M src/kudu/consensus/consensus.proto
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tserver/tablet_replica_lookup.h
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
M src/kudu/tserver/ts_tablet_manager.h
14 files changed, 503 insertions(+), 53 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/6772/8
-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.

Will Berkeley has posted comments on this change.

Change subject: KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................


Patch Set 7:

(54 comments)

http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/consensus/consensus.proto
File src/kudu/consensus/consensus.proto:

Line 471:   // The ids of the tablets.
> Can we add: An empty list means return info for all tablets known to the ta
Done


PS7, Line 557:   // Returns the consensus state for a tablet.
             :   rpc GetConsensusState(GetConsensusStateRequestPB) returns (GetConsensusStateResponsePB);
> Can we delete this function now? Gotta love deleting code. I don't think th
Done


Line 560:   // Returns the consensus state for a set of tablets.
> I think we should document whether this returns info for tombstoned tablets
Done


PS7, Line 561: GetConsensusStates
> I think GetConsensusState() is still a valid name for this endpoint, or if 
I like GetConsensusState.


http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:

Line 70:       const std::string& tablet_id,
> warning: parameter 'tablet_id' is unused [misc-unused-parameters]
Done


Line 71:       const Schema& schema,
> warning: parameter 'schema' is unused [misc-unused-parameters]
Done


PS7, Line 232:   
> nit: spacing
Done


PS7, Line 236: InsertOrDie
> Do we also need to worry about the duplicate UUID issue here?
No. This is test code that's manually setting up a vanilla consensus configuration on each mock TS.

If you wanted to test a case like that, you'd reach in and set it up in the specific test, as I do in the tests below for various situations


Line 243:                            string tablet_id,
> warning: the parameter 'tablet_id' is copied for each invocation but only u
Done


Line 243:                            string tablet_id,
> I think this is complaining because we should either specify the input argu
Kept it simple for the test and went with const string&.


Line 394:   FLAGS_check_consensus_info = false;
> No need to reset your flags between tests due to KuduTest::flag_saver_
:)


PS7, Line 401: shared_ptr<KsckTabletServer> ts
> is it safe to declare this const shared_ptr<KsckTabletServer>& ts ?
Done


PS7, Line 401: "ts-id-0")
> can we make "ts-id-0" and other identifier strings in here (except perhaps 
I don't think making constants for the whole file or for KsckTest makes sense just to cover these three specific tests, and the literals are actually used just once per test, so I don't think it makes it clearer or easier to store them in a variable.

FWIW, it's also common to use a literal "ts-id-0" or similar in other tests in this file.


Line 413:   FLAGS_check_consensus_info = false;
> also here
Done


Line 432:   FLAGS_check_consensus_info = false;
> and here
Done


Line 451:   FLAGS_check_consensus_info = false;
> and here
Done


Line 453: 
> Would also be interesting to test the duplicate TS UUID case.
You mean the rare + bad case from L123 of ksck_remote.cc? That code path can't be tested here as that situation is dealt with by the RemoteKsckTabletServer, not by Ksck.

What would happen, though, is that one of the duplicates' (same ts_id and tablet id) consensus info would be left out of the info provided to Ksck. I think it'd complicate the code to cover that rare edge case nicely, but it is easy to spit out a warning if we run into a duplicate. I'll do that.


http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

Line 63: using std::left;
> warning: using decl 'left' is unused [misc-unused-using-decls]
Done


PS7, Line 623: peers_from_master
> nit: peer_uuids_from_master might be clearer since when I read peer I think
Done


PS7, Line 624:   std::transform(tablet->replicas().cbegin(), tablet->replicas().cend(),
             :                  peers_from_master.begin(),
             :                  [](const std::shared_ptr<KsckTabletReplica>& r) { return r->ts_uuid(); });
> nit: This could be more simply written as a for-each loop:
Done


PS7, Line 627: boost::none
> Doesn't the master know the OpId index?
In its cmeta for the tablet I would guess so but that's not actually info that ksck gathers presently. KsckRemoteMaster is sneaky and gets its info about the master's POV on tablets by requesting scan tokens for a scan it'll never do. We'd have to refactor a fair bit more to add in the term/opid.


PS7, Line 627: -1
> nit: magic number. Also, doesn't the master know the term?
Magic number removed by using an enum. See above for why the master doesn't have the term here.


PS7, Line 649: strings::Substitute("$0:$1", ts->uuid(), tablet->id())
> as noted elsewhere this seems hacky compared to:
Done


PS7, Line 658: peers_from_replica
> same as above on naming
Done


PS7, Line 659:         std::transform(peers.begin(), peers.end(), peers_from_replica.begin(),
             :                        [](const consensus::RaftPeerPB& pb) { return pb.permanent_uuid(); });
> nit: same as above regarding functional vs. iterative but I guess it's also
Done


PS7, Line 693:     conflicting_states = std::count_if(replica_states.cbegin(), replica_states.cend(),
             :         [&](const ReplicaState& r) -> bool { return r.consensus_state != master_config; });
> nit: this is quite wordy compared to:
Done


Line 728:   } else if (conflicting_states > 0) {
> It seems quite easy to achieve this on a large cluster, which may be moving
I think that ksck "cries wolf" whenever there's nominal tablet movement going on in a cluster-- under-replication and consensus conflicts. Maybe later we can do a more thorough job of separating out meaningful warnings; for now, users can re-run ksck to see if the problem persists (targeting a specific table or tablet if needed).


PS7, Line 778: all_peers
> nit: peer_uuid_mapping?
Done


PS7, Line 795:     // A lambda to print out consensus from the point of view of the master and each replica.
             :     auto print_config = [&all_peers](const string& name,
             :                                      const boost::optional<KsckConsensusState>& config)
> Perhaps this lambda can be extracted into a function? Ksck::VerifyTablet() 
Done


Line 815:       if (config->term() >= 0) {
> Seems like term should be an optional
Done


PS7, Line 817: config->opid_index().get() >= 0
> Do we ever expect a negative number here? I don't think that's possible.
Done


PS7, Line 820:         Out() << Substitute("  $0.$1", config->term(), opid_str);
> I don't like this layout because this looks like this was the last log entr
Done


PS7, Line 834: "master"
> perhaps "config from master" to make this easier to understand?
Done


http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:

PS7, Line 97: bool pending
> can we make this an enum?
Good idea. I think if we have three types: MASTER, COMMITTED, and PENDING, it should make the class easier to understand.


Line 109:   bool pending() const {
> Needs API docs on the methods in this class, just a one liner like:
N/A. Removed method


PS7, Line 117: boost::optional<int64_t>
> const boost::optional<int64_t>& ?
N/A. Removed method


PS7, Line 121: const boost::optional<std::string> 
> const boost::optional<std::string>& ?
N/A. Removed method


Line 129:   bool operator==(const KsckConsensusState &other) const {
> Would be useful to doc this equality operator
Done


Line 130:     bool same_except_term = leader_uuid_ == other.leader_uuid_ && peers_ == other.peers_;
> why not check pending_?
pending_ removed in favor of enum type; type is now checked


Line 131:     if (term_ >= 0 && other.term_ >= 0) {
> why not just term_ == other.term_ ?
N/A with addition of MASTER type


Line 137:  private:
> It looks like we can make all of these member variables const because they'
I made it struct since that seems simpler. I want the copy assign operator because of ksck's logic of building a replica state one bit at a time, so I didn't make the fields const.


Line 139:   int64_t term_;
> Since we are using a magic number -1 somewhere in ksck.cc maybe we should m
Done


PS7, Line 142: peers_
> Not sure what this is. How about peer_uuids_ ?
Done


PS7, Line 142: std::set
> I guess using a std::set here saves us a sort if we want to print these pee
Yup. That's the idea.


PS7, Line 238: std::string
> Hmm, I think it would be a little cleaner if we keyed this map by std::pair
Done


PS7, Line 247: FetchConsensusInfo
> nit: Maybe FetchConsensusState() would be more clear
Done


PS7, Line 444: CONFLICTED
> nit: maybe CONSENSUS_MISMATCH? Also add a comment to document its meaning.
Done


http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

Line 116:   for (const auto& entry : tablet_status_map_) {
> Is there a reason we don't just ask for all of them?
No reason. My bad-- ksck lets you filter which tables to look at, but that is done at a higher level than this, so we will always gather all the info here.


PS7, Line 122: Don't crash
> Seems like we should warn in this case? Or perhaps detect this and warn els
Came to the same conclusion while looking at another one of your comments before I saw this one. Done.


http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/tools/ksck_remote.h
File src/kudu/tools/ksck_remote.h:

PS7, Line 58: FetchConsensusInfo
> nit: Maybe FetchConsensusState()?
Done. I also renamed the flag from check_consensus_info to check_consensus_state.


http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/tools/tool_action_cluster.cc
File src/kudu/tools/tool_action_cluster.cc:

PS7, Line 138: check_consensus_info
> how about just --consensus ?
Done


http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:

PS7, Line 1067: req->tablet_ids()
> Is this is empty we can get them all via tablet_manager_->GetTabletReplicas
Done


Line 1069:     if (!tablet_manager_->GetTabletReplica(tablet_id, &tablet_replica).ok()) {
> As mentioned elsewhere, this excludes tombstoned tablets. I actually think 
I agree we should leave it out and perhaps add something in later to get it later.


PS7, Line 1079:     ConsensusStatePB state = consensus->ConsensusState();
              :     tablet_info->mutable_cstate()->Swap(&state);
> do this in one line like:
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.

Will Berkeley has uploaded a new patch set (#2).

Change subject: [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................

[WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Sample output:
1. https://gist.github.com/wdberkeley/aa97e9a108b57acacc4da6db0625445b
2. (problem #1) https://gist.github.com/wdberkeley/b8f9987caf8bd15ec4fd8fd345a217f9

WIP because it needs tests.

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/6772/2
--
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.

Will Berkeley has posted comments on this change.

Change subject: KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

PS7, Line 627: -1
> Magic number removed by using an enum. See above for why the master doesn't
s/see above/see response to other comment on this line/


http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/tools/ksck_remote.h
File src/kudu/tools/ksck_remote.h:

PS7, Line 58: FetchConsensusInfo
> Done. I also renamed the flag from check_consensus_info to check_consensus_
And then just to consensus


-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.

Will Berkeley has posted comments on this change.

Change subject: KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................


Patch Set 4:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/6772/5/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:

Line 70:       const Schema& schema,
> warning: parameter 'tablet_id' is unused [misc-unused-parameters]
Expected


Line 71:       const ChecksumOptions& options,
> warning: parameter 'schema' is unused [misc-unused-parameters]
Expected


Line 242:   Status RunKsck() {
> warning: non-const reference parameter 'replicas', make it const or use a p
Done


http://gerrit.cloudera.org:8080/#/c/6772/5/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

Line 798: } // namespace kudu
> error: cannot initialize a parameter of type 'FILE *' (aka '_IO_FILE *') wi
Done


Line 798: } // namespace kudu
> error: use of undeclared identifier 'setw'; did you mean 'getw'? [clang-dia
Done


Line 798: } // namespace kudu
> error: use of undeclared identifier 'setw'; did you mean 'getw'? [clang-dia
Done


http://gerrit.cloudera.org:8080/#/c/6772/5/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:

Line 104:     return term_;
> warning: value argument 'opid_index' can be moved to avoid copy [misc-move-
Done


http://gerrit.cloudera.org:8080/#/c/6772/5/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:

Line 115: using kudu::consensus::LeaderStepDownResponsePB;
> warning: using decl 'RaftConfigPB' is unused [misc-unused-using-decls]
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.

Will Berkeley has posted comments on this change.

Change subject: KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................


Patch Set 8:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/6772/8/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:

Line 72:       const ChecksumOptions& options,
> warning: parameter 'options' is unused [misc-unused-parameters]
Done


http://gerrit.cloudera.org:8080/#/c/6772/8/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

Line 66: using std::setfill;
> warning: using decl 'setfill' is unused [misc-unused-using-decls]
Done


http://gerrit.cloudera.org:8080/#/c/6772/8/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:

Line 113:       term(term),
> warning: value argument 'term' can be moved to avoid copy [misc-move-constr
Done


http://gerrit.cloudera.org:8080/#/c/6772/8/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:

Line 109: using kudu::consensus::ConsensusStatePB;
> warning: using decl 'ConsensusStatePB' is unused [misc-unused-using-decls]
Done


Line 1056:   for (const scoped_refptr<TabletReplica> replica : tablet_replicas) {
> warning: the loop variable's type is not a reference type; this creates a c
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................


Patch Set 7:

(50 comments)

In your example output, this is shown:

  The consensus matrix is:
  master: A     B*    C
       A: [no config retrieved]
       B: A     B*          38.33 pending
       C: A     B     C     38.--

However I think this should be impossible to achieve because a pending config doesn't have an opid_index and a committed one must always have one.

http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/consensus/consensus.proto
File src/kudu/consensus/consensus.proto:

Line 471:   // The ids of the tablets.
Can we add: An empty list means return info for all tablets known to the tablet server.


PS7, Line 557:   // Returns the consensus state for a tablet.
             :   rpc GetConsensusState(GetConsensusStateRequestPB) returns (GetConsensusStateResponsePB);
Can we delete this function now? Gotta love deleting code. I don't think this RPC endpoint has been used outside of tests in any released version of Kudu. Since ksck won't use it either, can we just migrate / bridge the existing test callsites to GetConsensusStates()?

From a quick git grep for GetConsensusState I only see 3 callsites (all from tests) and they all of which use the helper function kudu::itest::GetConsensusState() in src/kudu/integration-tests/cluster_itest_util.cc so you could just change that function to use the new RPC endpoint to make this dead code.


Line 560:   // Returns the consensus state for a set of tablets.
I think we should document whether this returns info for tombstoned tablets. Right now it doesn't, because they are filtered by the tablet manager. Should it?


PS7, Line 561: GetConsensusStates
I think GetConsensusState() is still a valid name for this endpoint, or if you don't want to reuse the name perhaps GetConsensusStateBatch()


http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:

PS7, Line 232:   
nit: spacing


PS7, Line 236: InsertOrDie
Do we also need to worry about the duplicate UUID issue here?


Line 243:                            string tablet_id,
> warning: the parameter 'tablet_id' is copied for each invocation but only u
I think this is complaining because we should either specify the input argument as const string& or we should be moving on L259, i.e. InsertOrDie(&ts->tablet_status_map_, std::move(tablet_id), std::move(pb)); although I don't recall whether InsertOrDie() is actually move optimized (the underlying map is).


Line 394:   FLAGS_check_consensus_info = false;
No need to reset your flags between tests due to KuduTest::flag_saver_


PS7, Line 401: shared_ptr<KsckTabletServer> ts
is it safe to declare this const shared_ptr<KsckTabletServer>& ts ?


PS7, Line 401: "ts-id-0")
can we make "ts-id-0" and other identifier strings in here (except perhaps "ts-id_fake") a constant?


Line 413:   FLAGS_check_consensus_info = false;
also here


Line 432:   FLAGS_check_consensus_info = false;
and here


Line 451:   FLAGS_check_consensus_info = false;
and here


Line 453: 
Would also be interesting to test the duplicate TS UUID case.


http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

PS7, Line 623: peers_from_master
nit: peer_uuids_from_master might be clearer since when I read peer I think of consensus_peers.cc or other classes


PS7, Line 624:   std::transform(tablet->replicas().cbegin(), tablet->replicas().cend(),
             :                  peers_from_master.begin(),
             :                  [](const std::shared_ptr<KsckTabletReplica>& r) { return r->ts_uuid(); });
nit: This could be more simply written as a for-each loop:

  for (const auto& replica : tablet->replicas()) {
    peers_from_master.push_back(replica->ts_uuid());
  }


PS7, Line 627: -1
nit: magic number. Also, doesn't the master know the term?


PS7, Line 627: boost::none
Doesn't the master know the OpId index?


PS7, Line 649: strings::Substitute("$0:$1", ts->uuid(), tablet->id())
as noted elsewhere this seems hacky compared to:

  auto tablet_key = std::make_pair(ts->uuid(), tablet->id());


PS7, Line 658: peers_from_replica
same as above on naming


PS7, Line 659:         std::transform(peers.begin(), peers.end(), peers_from_replica.begin(),
             :                        [](const consensus::RaftPeerPB& pb) { return pb.permanent_uuid(); });
nit: same as above regarding functional vs. iterative but I guess it's also a matter of personal taste


PS7, Line 693:     conflicting_states = std::count_if(replica_states.cbegin(), replica_states.cend(),
             :         [&](const ReplicaState& r) -> bool { return r.consensus_state != master_config; });
nit: this is quite wordy compared to:

  for (const auto& r : replica_states) {
    if (r.consensus_state != master_config) conflicting_states++;
  }


Line 728:   } else if (conflicting_states > 0) {
It seems quite easy to achieve this on a large cluster, which may be moving tablets on a regular basis. I wonder if we should try to avoid marking things yellow if they've only just changed. We don't currently store the age of the config but maybe we should (in memory, with max age == the uptime of the process). Then maybe only consider it a failure if the TS says it's had that config for more than 10 seconds or something?


PS7, Line 778: all_peers
nit: peer_uuid_mapping?


PS7, Line 795:     // A lambda to print out consensus from the point of view of the master and each replica.
             :     auto print_config = [&all_peers](const string& name,
             :                                      const boost::optional<KsckConsensusState>& config)
Perhaps this lambda can be extracted into a function? Ksck::VerifyTablet() is already too long (several screen heights even on a very large monitor)


Line 815:       if (config->term() >= 0) {
Seems like term should be an optional


PS7, Line 817: config->opid_index().get() >= 0
Do we ever expect a negative number here? I don't think that's possible.


PS7, Line 820:         Out() << Substitute("  $0.$1", config->term(), opid_str);
I don't like this layout because this looks like this was the last log entry or something but it's not. In fact the OpId(config->term(), config->opid_index()) may not exist at all. That's because the opid_index of the config is likely from a previous term and due to leader churn config->term() is now way higher.

With the current layout, we could end up printing something like 10.0 for original configs that happen to be in term 10, which seems confusing.

I think it would make more sense to print this as two separate columns and add a header row to the ASCII output. How about something like this?


  The consensus matrix is:

  Host                 Voters                 Current term  Config index  Committed config?
  -------------------  ---------------------  ------------  ------------  -----------------
  config from master:  A     B*    C          ?             ?             Yes
                   A:  [no config retrieved]
                   B:  A     B*               38            33            Yes
                   C:  A     B     C          38            --            No


PS7, Line 834: "master"
perhaps "config from master" to make this easier to understand?


http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:

PS7, Line 97: bool pending
can we make this an enum?


Line 109:   bool pending() const {
Needs API docs on the methods in this class, just a one liner like:

  // Returns true if this represents a pending config.
  bool pending() const {


PS7, Line 117: boost::optional<int64_t>
const boost::optional<int64_t>& ?


PS7, Line 121: const boost::optional<std::string> 
const boost::optional<std::string>& ?


Line 129:   bool operator==(const KsckConsensusState &other) const {
Would be useful to doc this equality operator


Line 130:     bool same_except_term = leader_uuid_ == other.leader_uuid_ && peers_ == other.peers_;
why not check pending_?


Line 131:     if (term_ >= 0 && other.term_ >= 0) {
why not just term_ == other.term_ ?


Line 137:  private:
It looks like we can make all of these member variables const because they're only set in the constructor. I wonder if we should also just make this class a struct and get rid of the getters? We could still keep the constructor and the operator==. Just a thought.


Line 139:   int64_t term_;
Since we are using a magic number -1 somewhere in ksck.cc maybe we should make this an optional instead.


PS7, Line 142: peers_
Not sure what this is. How about peer_uuids_ ?


PS7, Line 142: std::set
I guess using a std::set here saves us a sort if we want to print these peers in alphabetical order?


PS7, Line 238: std::string
Hmm, I think it would be a little cleaner if we keyed this map by std::pair<std::string, std::string> since we're assuming <TS UUID, tablet id> as the key. Although I guess the other one should too.


PS7, Line 247: FetchConsensusInfo
nit: Maybe FetchConsensusState() would be more clear


PS7, Line 444: CONFLICTED
nit: maybe CONSENSUS_MISMATCH? Also add a comment to document its meaning.


http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

Line 116:   for (const auto& entry : tablet_status_map_) {
Is there a reason we don't just ask for all of them?


PS7, Line 122: Don't crash
Seems like we should warn in this case? Or perhaps detect this and warn elsewhere if we see multiple endpoints with the same UUID?


http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/tools/ksck_remote.h
File src/kudu/tools/ksck_remote.h:

PS7, Line 58: FetchConsensusInfo
nit: Maybe FetchConsensusState()?


http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/tools/tool_action_cluster.cc
File src/kudu/tools/tool_action_cluster.cc:

PS7, Line 138: check_consensus_info
how about just --consensus ?


http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:

PS7, Line 1067: req->tablet_ids()
Is this is empty we can get them all via tablet_manager_->GetTabletReplicas() by just adding it to TabletReplicaLookupIf since it's already implemented in TSTabletManager and it would be easy to add to CatalogManager.


Line 1069:     if (!tablet_manager_->GetTabletReplica(tablet_id, &tablet_replica).ok()) {
As mentioned elsewhere, this excludes tombstoned tablets. I actually think that's what we want in most cases but I'm not sure if there is a case where we want that information remotely for debugging purposes, since it's potentially going to represent some old config and whether it voted in the most recent election once we have tombstoned voting.

After writing all this out, I think we should probably just leave it as-is (exclude tombstones) and if we find a need for it later we can add an additional optional parameter to get it (the response size would be way bigger too).


PS7, Line 1079:     ConsensusStatePB state = consensus->ConsensusState();
              :     tablet_info->mutable_cstate()->Swap(&state);
do this in one line like:

  *tablet_info->mutable_cstate() = consensus->ConsensusState();


-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................


Patch Set 11: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

[kudu-CR] [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/6772

to look at the new patch set (#3).

Change subject: [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................

[WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Sample output:
1. https://gist.github.com/wdberkeley/aa97e9a108b57acacc4da6db0625445b
2. (problem #1) https://gist.github.com/wdberkeley/b8f9987caf8bd15ec4fd8fd345a217f9

WIP because it needs tests.

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/6772/3
--
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................


Patch Set 4:

> I implemented a GetAllConsensusStates RPC and used it instead of
> one GetConsensusState RPC per tablet replica

I think it would be better to fully replace it with the new RPC because if you do a search it's not used anywhere except tests.

> However, this makes the new features available only for
> servers >= 1.4. Should we stick both forms of logic in
> so it works on older servers?

I don't think it's important for it to work on older servers since it's a new command.

-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

[kudu-CR] [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6772/1/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

Line 716:   } else if (consensus_conflict) {
> Yes, that's part of a core problem with these consensus checks: we can't re
I think it would be useful to include stats about how long the config has been pending if there is a pending config. It shouldn't be too hard to add. However I agree that using comparisons to decide whether there is a problem is hard to get right.

Another thing to consider for a tablet health check would be adding a no-op canary.

I agree allowing all info to be printed wouldn't necessarily be a bad thing, even if we don't flag it as an error but potentially as a warning.


http://gerrit.cloudera.org:8080/#/c/6772/1/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:

PS1, Line 124:   int64_t term_;
             :   boost::optional<string> leader_uuid_;
             :   vector<string> peers_;
> If you make them const it breaks the assignment at ksck.cc L663 since the c
k


-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6772

to look at the new patch set (#5).

Change subject: KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................

KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

This patch enhances ksck to gather consensus info from every tablet. It
compares this info with master and outputs the master's config and every
conflicting config, if there are any conflicts. To do this efficiently
it implements a new GetAllConsensusStates RPC that gathers info about
every replica's consensus state.

This will catch at least the two problems identified in KUDU-1860:
1. The leader has a pending config to remove a tablet, but it is not
committed so the master does not see this config. This can hide an
unhealthy tablet if, e.g., one pending config member is down and the
pending-to-be-kicked-out member is up, so 1/2 replicas are alive in
the leader's active config but the master thinks 2/3 are alive.
2. No replica is leader but the master believes there is a leader
because its cache is old and hasn't been updated.

Sample output showing #1:
https://gist.github.com/wdberkeley/f964439bb0c407d3769c2dd9a0959ca0

Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
---
M src/kudu/consensus/consensus.proto
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
9 files changed, 408 insertions(+), 21 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/6772/5
-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................


Patch Set 9:

(8 comments)

Looking good, just a few more fairly minor comments

http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:

Line 453:       "  config from master:  A*  B   C                                 Yes       \n"
> You mean the rare + bad case from L123 of ksck_remote.cc? That code path ca
Sounds good. I'm just wondering if we can make a test case for that scenario to make sure ksck warns and doesn't crash.


http://gerrit.cloudera.org:8080/#/c/6772/9/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:

Line 396: TEST_F(KsckTest, TestConsensusConflictExtraPeer) {
Can we also add a test case with Committed == no?

You could pretty easily do that in a config of 3 by killing the 2 followers and then asking the leader to evict one of the followers using itest::RemoveServer()


http://gerrit.cloudera.org:8080/#/c/6772/9/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

PS9, Line 604: ReplicaState
nit: how about ReplicaInfo since we have an important consensus class called consensus::ReplicaState.


Line 612: // Formatting constants
nit: Your code comments should all end with punctuation per https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_Grammar so this one and all the others added should end with a period. See L616, L674, and others as well.


Line 672:   const string no_config = "[no config retrieved";
not used? also missing ending ]


http://gerrit.cloudera.org:8080/#/c/6772/9/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:

Line 123:   bool operator==(const KsckConsensusState &other) const {
When I see operator== I think that it's only going to return true for structs that are exactly equal. operator== is also the default equality indicator used in the STL for things like std::unordered_map<>.

Since this returns true for consensus states that aren't truly equal I think it would be less surprising to give this method a name such as Matches() or something, which would lead me to read the header comment to understand the semantics.


http://gerrit.cloudera.org:8080/#/c/6772/9/src/kudu/tserver/tablet_replica_lookup.h
File src/kudu/tserver/tablet_replica_lookup.h:

Line 54:   virtual void GetTabletReplicas(
Add comment: Appends all non-tombstoned tablet replicas to the 'replicas' vector.


http://gerrit.cloudera.org:8080/#/c/6772/9/src/kudu/tserver/tablet_service.h
File src/kudu/tserver/tablet_service.h:

Line 183:                                      consensus::GetConsensusStateResponsePB* resp,
nit: Indentation should look like GetLastOpId()


-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6772

to look at the new patch set (#6).

Change subject: KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................

KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

This patch enhances ksck to gather consensus info from every tablet. It
compares this info with master and outputs the master's config and every
conflicting config, if there are any conflicts. To do this efficiently
it implements a new GetAllConsensusStates RPC that gathers info about
every replica's consensus state.

This will catch at least the two problems identified in KUDU-1860:
1. The leader has a pending config to remove a tablet, but it is not
committed so the master does not see this config. This can hide an
unhealthy tablet if, e.g., one pending config member is down and the
pending-to-be-kicked-out member is up, so 1/2 replicas are alive in
the leader's active config but the master thinks 2/3 are alive.
2. No replica is leader but the master believes there is a leader
because its cache is old and hasn't been updated.

Sample output showing #1:
https://gist.github.com/wdberkeley/f964439bb0c407d3769c2dd9a0959ca0

Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
---
M src/kudu/consensus/consensus.proto
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
9 files changed, 412 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/6772/6
-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.

Todd Lipcon has posted comments on this change.

Change subject: [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................


Patch Set 4:

Just passing through, haven't looked at the code yet, but one thought: to make the output more readable, maybe we can assign a letter or number to each of the peers in a config? Something like:


WARNING: Some replicas' configs differ from the master's.
The master's config is:
    4414b7f80f4d488eb65e7772759f2bcf (A)
    9e8acf07e76c468eadaf1903e923a86a (B) [LEADER]
    ac2de1a8f62542fcb7a180dc232b1c30 (C)
The following replicas' configs disagree with the master's or could not be retrieved:
  Replica 9e8acf07e76c468eadaf1903e923a86a (B) with config
    9e8acf07e76c468eadaf1903e923a86a (B) [LEADER]
    ac2de1a8f62542fcb7a180dc232b1c30 (C)
  Replica 4414b7f80f4d488eb65e7772759f2bcf (A) did not return a config
  Replica ac2de1a8f62542fcb7a180dc232b1c30 (C) did not return a config


should make it a bit easier to read at a glance

-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

[kudu-CR] [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.

Will Berkeley has posted comments on this change.

Change subject: [WIP] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................


Patch Set 2:

(1 comment)

Lint fixed.

http://gerrit.cloudera.org:8080/#/c/6772/2/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:

Line 100:       peers_(peers) {
> warning: value argument 'peers' can be moved to avoid copy [misc-move-const
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has submitted this change and it was merged.

Change subject: KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................


KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

This patch enhances ksck to gather consensus info from every
tablet. It compares this info with master and outputs the
master's config and every conflicting config, if there are any
conflicts. To do this efficiently it reimplements the
GetAllConsensusState RPC so that it gathers info about every
replica's consensus state.

This will catch at least the two problems identified in
KUDU-1860: 1. The leader has a pending config to remove a
tablet, but it is not committed so the master does not see this
config. This can hide an unhealthy tablet if, e.g., one pending
config member is down and the pending-to-be-kicked-out member is
up, so 1/2 replicas are alive in the leader's active config but
the master thinks 2/3 are alive. 2. No replica is leader but the
master believes there is a leader because its cache is old and
hasn't been updated.

Sample output showing #1:
https://gist.github.com/wdberkeley/d2606698e4f2e8ca3ef70d4dcef7ba9a

Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Reviewed-on: http://gerrit.cloudera.org:8080/6772
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/consensus/consensus.proto
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tserver/tablet_replica_lookup.h
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
M src/kudu/tserver/ts_tablet_manager.h
14 files changed, 521 insertions(+), 71 deletions(-)

Approvals:
  Mike Percy: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6772

to look at the new patch set (#9).

Change subject: KUDU-1860: ksck doesn't identify tablets that are evicted but still in config
......................................................................

KUDU-1860: ksck doesn't identify tablets that are evicted but still in config

This patch enhances ksck to gather consensus info from every
tablet. It compares this info with master and outputs the
master's config and every conflicting config, if there are any
conflicts. To do this efficiently it reimplements the
GetAllConsensusState RPC so that it gathers info about every
replica's consensus state.

This will catch at least the two problems identified in
KUDU-1860: 1. The leader has a pending config to remove a
tablet, but it is not committed so the master does not see this
config. This can hide an unhealthy tablet if, e.g., one pending
config member is down and the pending-to-be-kicked-out member is
up, so 1/2 replicas are alive in the leader's active config but
the master thinks 2/3 are alive. 2. No replica is leader but the
master believes there is a leader because its cache is old and
hasn't been updated.

Sample output showing #1:
https://gist.github.com/wdberkeley/d2606698e4f2e8ca3ef70d4dcef7ba9a

Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
---
M src/kudu/consensus/consensus.proto
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tserver/tablet_replica_lookup.h
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
M src/kudu/tserver/ts_tablet_manager.h
14 files changed, 502 insertions(+), 54 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/6772/9
-- 
To view, visit http://gerrit.cloudera.org:8080/6772
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16e4de09821b372c3773b4ade3fd9e37ab818808
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>