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 <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Tue, 10 Apr 2018 09:30:48 +0000 Gerrit-HasComments: Yes
