Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9787 )
Change subject: KUDU-2364 Add extra check in ksck for tserver ID ...................................................................... Patch Set 7: (10 comments) http://gerrit.cloudera.org:8080/#/c/9787/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9787/2//COMMIT_MSG@21 PS2, Line 21: 2ff411da5bbe177631ace5d > I'm not sure how this should work. Currently ksck counts the number of !s.o You could make a TabletServerErrors struct or something, and tabulate the OKs and other errors in it, along with uuid/hostport, then print those out as a small table at the end. I'd say it should go above the tablet summary table since it's usually less interesting. http://gerrit.cloudera.org:8080/#/c/9787/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9787/7//COMMIT_MSG@10 PS7, Line 10: TabletServer nit: "tablet server" is fine. http://gerrit.cloudera.org:8080/#/c/9787/7/src/kudu/server/server_base.proto File src/kudu/server/server_base.proto: http://gerrit.cloudera.org:8080/#/c/9787/7/src/kudu/server/server_base.proto@38 PS7, Line 38: optional bytes uuid = 5; The NodeInstancePB already contains the uuid. See common/wire_protocol.proto. http://gerrit.cloudera.org:8080/#/c/9787/7/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: http://gerrit.cloudera.org:8080/#/c/9787/7/src/kudu/tools/ksck-test.cc@327 PS7, Line 327: /* Oops, looks like this test is incomplete and commented out. You'll want to make a function like CreateClusterWithMismatchedTSUuid that reaches into the KsckCluster object and replaces one of its tablet servers with one with a different uuid, and replace CreateOneSmallReplicatedTable() with a call to that function. http://gerrit.cloudera.org:8080/#/c/9787/7/src/kudu/tools/ksck-test.cc@335 PS7, Line 335: TabletServer Just "tablet server". I know ksck says "Tablet Server" in a lot of places but I think the capitalization is a mistake and I'll be changing it whenever I add to or modify ksck. http://gerrit.cloudera.org:8080/#/c/9787/5/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: http://gerrit.cloudera.org:8080/#/c/9787/5/src/kudu/tools/ksck_remote-test.cc@231 PS5, Line 231: tserver::MiniTabletServer* tablet_server = mini_cluster_->mini_tablet_server(0); I think it'd be nicer to implement a method like ExternalMiniCluster::DeleteFromDisk for InternalMiniCluster daemons and use that. As an side, I think we should be using the ExternalMiniCluster for these tests anyway since they should be black-box. I'll have to put that on my todo. http://gerrit.cloudera.org:8080/#/c/9787/5/src/kudu/tools/ksck_remote-test.cc@242 PS5, Line 242: ASSERT_TRUE(!s.ok()); Can you add a check that the expected + actual tserver uuids appear in the message? They should be available as methods on the minicluster daemon objects. http://gerrit.cloudera.org:8080/#/c/9787/2/src/kudu/tools/ksck_remote.cc File src/kudu/tools/ksck_remote.cc: http://gerrit.cloudera.org:8080/#/c/9787/2/src/kudu/tools/ksck_remote.cc@116 PS2, Line 116: { > ok, moved this check for client side. Should I leave the uuid check in the Nope. http://gerrit.cloudera.org:8080/#/c/9787/5/src/kudu/tools/ksck_remote.cc File src/kudu/tools/ksck_remote.cc: http://gerrit.cloudera.org:8080/#/c/9787/5/src/kudu/tools/ksck_remote.cc@109 PS5, Line 109: std:: The namespace qualifier isn't needed. http://gerrit.cloudera.org:8080/#/c/9787/5/src/kudu/tools/ksck_remote.cc@121 PS5, Line 121: req.set_need_schema_info(f If we're doing the check in GetStatus I think we should remove it here and not put the uuid in ListTablets. Maybe at a later date we'll make most RPC endpoints optionally check uuid. ListTablets can wait for that patch. -- To view, visit http://gerrit.cloudera.org:8080/9787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia2c18ba7af8eaa6f5e4d7842f18754d2c1e32526 Gerrit-Change-Number: 9787 Gerrit-PatchSet: 7 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Tue, 27 Mar 2018 17:50:56 +0000 Gerrit-HasComments: Yes
