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 <[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
