Attila Bukor 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: (8 comments) 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. Done 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.prot thanks, removing it 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: /* > I think the test on the previous file does that via starting up a new tserv yep, I committed this test by mistake, removing it 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 b Done 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::Delet given that we're using InternalMiniCluster everywhere else in ksck_remote-test I assume I should keep this on InternalMiniCluster as well, right? If it stays on InternalMiniCluster, shouldn't it be InternalMiniCluster::DeleteFromDisk instead()? 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 Done http://gerrit.cloudera.org:8080/#/c/9787/7/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: http://gerrit.cloudera.org:8080/#/c/9787/7/src/kudu/tools/ksck_remote-test.cc@232 PS7, Line 232: std::string("2") > I think you should be able to simplify this to thanks, don't know why I used std::string("2") here... http://gerrit.cloudera.org:8080/#/c/9787/7/src/kudu/tools/ksck_remote.cc File src/kudu/tools/ksck_remote.cc: http://gerrit.cloudera.org:8080/#/c/9787/7/src/kudu/tools/ksck_remote.cc@122 PS7, Line 122: req.set_dest_uuid(uuid()); > Is it necessary to add the new UUID check in ListTablets now that you also removing it, thanks -- 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 19:26:09 +0000 Gerrit-HasComments: Yes
