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 <[email protected]>
Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <[email protected]>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <[email protected]>
Gerrit-Reviewer: Will Berkeley <[email protected]>
Gerrit-HasComments: Yes
