Will Berkeley 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: (8 comments) Both of your comments about more testing are things that aren't done well by the current ksck tests. I think the takeaway is there should by a ksck- or tool-itest to cover these situations. It would also be useful as a place to see what situations ksck should detect. http://gerrit.cloudera.org:8080/#/c/6772/7/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: Line 453: > Sounds good. I'm just wondering if we can make a test case for that scenari Hm you'd have to actually get the RPC to return duplicates from 1 or more ts. Another reason to add a tool itest like I mentioned in another comment. http://gerrit.cloudera.org:8080/#/c/6772/9/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: Line 396: > Can we also add a test case with Committed == no? This isn't an integration test. I went to go find the tool-itest I thought would exist and there isn't one. Looks like a todo: add a tool-itest or a ksck-itest. http://gerrit.cloudera.org:8080/#/c/6772/9/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: PS9, Line 604: string tabl > nit: how about ReplicaInfo since we have an important consensus class calle Done Line 612: boost::optional<tablet::TabletStatusPB> status_pb; > nit: Your code comments should all end with punctuation per https://google. Done Line 672: repl_leader_uuid, > not used? also missing ending ] Done http://gerrit.cloudera.org:8080/#/c/6772/9/src/kudu/tools/ksck.h File src/kudu/tools/ksck.h: Line 123: } > When I see operator== I think that it's only going to return true for struc Done 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: > Add comment: Appends all non-tombstoned tablet replicas to the 'replicas' v Done 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() Fixed all the off indents in this section. -- 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 <wdberke...@gmail.com> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-HasComments: Yes
