Andrew Wong 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: (10 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 get into this state IIUC, the operator specified masters A, B, C, but the actual configs point to A, B, D. 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 "Expected a single leader but got multiple"? 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<>? 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? 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 rest of the patch, but right now, I'm finding this a little confusing. "Return a formatted stringified version of 'config', mapping UUIDs to single-character labels with the mapping 'uuid_mapping'." nit: Also not sure if this is overkill but maybe rename 'uuid_mapping' to 'label_by_uuid' or something? Ah I now see that you just moved this from down below, so feel doubly inclined to disregard if you so please. 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 common pattern, maybe consider: struct label_and_pair { char label; string replica_uuid; } ...so that you can refer to them below as e.replica_uuid and e.label instead of first/second. Although I don't feel super strongly about either name change so feel free to ignore. Ah I now see that you just moved this from down below, so feel doubly inclined to disregard if you so please. 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 an empty string as a leader_uuid? 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? http://gerrit.cloudera.org:8080/#/c/9883/5/src/kudu/tools/ksck.cc@293 PS5, Line 293: uuid If we don't already have this listed somewhere, maybe adding `master->address()` would be beneficial for known UUIDs as well. At least basing off of the sample output in the commit message, it'd probably be helpful to list the UUID --> hostport mapping if we don't. Although since you're keying on it below, maybe the extra labeling belongs somewhere else. 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 UUID for unavailable masters? With that, I think we'd be able to keep this block of code the same and avoid an extra step in CheckMasterConsensus(). -- 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: Mon, 09 Apr 2018 19:04:44 +0000 Gerrit-HasComments: Yes
