Will Berkeley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9883 )

Change subject: [tools] ksck improvements [3/n]: master consensus checks
......................................................................


Patch Set 5: -Verified

(11 comments)

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

http://gerrit.cloudera.org:8080/#/c/9883/5/src/kudu/tools/ksck-test.cc@409
PS5, Line 409:
             : TEST_F(KsckTest, TestWrongMasterUuid) {
> nit: maybe add a comment explaining an example of when we might expect to g
Done


http://gerrit.cloudera.org:8080/#/c/9883/5/src/kudu/tools/ksck-test.cc@454
PS5, Line 454:     " Config source |   Replicas   | Current term | Config index 
| Committed?\n"
             :     
"---------------+--------------+--------------+--------------+------------\n"
             :     " A             | A*  B   C    | 0            |              
| Yes\n"
             :     " B             | A   B*  C    | 0            |              
| Yes\n"
             :     " C             | A*  B   C    | 0            |              
| Yes\n");
> nit: Do you think it's worth adding any additional info here, like "Expecte
I think that's clear enough from the consensus matrix. Pointing it out 
especially doesn't really change or help anything, I don't think.


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

http://gerrit.cloudera.org:8080/#/c/9883/5/src/kudu/tools/ksck.h@121
PS5, Line 121:   KsckConsensusState() = default;
> Is this needed so we can wrap boost::optional<>?
No, it's needed because there's also an explicit constructor, so the compiler 
won't implicitly implement a 0-arg constructor.


http://gerrit.cloudera.org:8080/#/c/9883/5/src/kudu/tools/ksck.h@305
PS5, Line 305:   boost::optional<consensus::ConsensusStatePB> cstate_;
> nit: doc when might this be none?
Done


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

http://gerrit.cloudera.org:8080/#/c/9883/5/src/kudu/tools/ksck.cc@102
PS5, Line 102: // Format replicas known and unknown to 'config' using labels 
from 'uuid_mapping'.
> nit: Hmm maybe I'll be able to parse this better after reading through the
Done


http://gerrit.cloudera.org:8080/#/c/9883/5/src/kudu/tools/ksck.cc@107
PS5, Line 107: replicas
> nit: similarly, maybe label_and_replica_pair or something? Or if this is a
Howabout labeled_replica as a name for the pair? Making a struct out of it is 
appealing but I think the scope of usage is small enough it's easier just to 
use a std::pair.


http://gerrit.cloudera.org:8080/#/c/9883/5/src/kudu/tools/ksck.cc@134
PS5, Line 134:   ksck_cstate->type = cstate.has_pending_config() ?
> shouldn't these two be grouped under a single
I wouldn't be able to declare config as const in that case :(

Beyond that, doesn't seem to matter much which way it's done. If you really 
have a strong pref I can change it.


http://gerrit.cloudera.org:8080/#/c/9883/5/src/kudu/tools/ksck.cc@143
PS5, Line 143: !cstate.leader_uuid().empty()
> Is this different than cstate.has_leader_uuid()? When might the cstate have
If memory serves, the reason it's like this is because Kudu servers set 
leader_uuid to "" explicitly when there's no leader, so we test for emptiness 
instead of non-presence.

I added a comment explaining this.


http://gerrit.cloudera.org:8080/#/c/9883/5/src/kudu/tools/ksck.cc@147
PS5, Line 147:   vector<string> voter_uuids;
             :   vector<string> non_voter_uuids;
> Why this extra buffering instead of inserting directly into the sets?
Good catch. Not sure. Maybe it was part of a function that could fail in the 
past? Changed it.


http://gerrit.cloudera.org:8080/#/c/9883/5/src/kudu/tools/ksck.cc@293
PS5, Line 293: uuid
> I agree and based on the usages below I believe this method could return uu
I agree this would make the code a little clearer but I think we should leave 
it for the moment. See my response to one of Andrew's comment below.


http://gerrit.cloudera.org:8080/#/c/9883/5/src/kudu/tools/ksck.cc@461
PS5, Line 461: The address is used in the sort for the unavailable master case, 
because
             :   // we do not know the uuid in that case.
> How feasible would it be to assign the "kDummyId (hostport)" string as the
Ah this sort of thing might be nice but I think it'd mess up some scripts we 
commonly use to fix broken tablets :(. The n in [3/n], [4/n],... will 
eventually get big enough to see a patch where ksck can auto-repair these 
common broken situations so we can change the output more freely.

Keep in mind also that, in both the master and tserver case, the addresses of 
the nodes are listed just above the consensus matrix part, so we'd be repeating 
ourselves a bit.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1015854759debb3f1acf4bf9eb143260547f4a4b
Gerrit-Change-Number: 9883
Gerrit-PatchSet: 5
Gerrit-Owner: Will Berkeley <wdberke...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <abu...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com>
Gerrit-Comment-Date: Tue, 10 Apr 2018 09:30:48 +0000
Gerrit-HasComments: Yes

Reply via email to