Dan Burkert 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: (3 comments) 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. I think the test on the previous file does that via starting up a new tserver bound to the same RPC addresses. 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 string root_dir = mini_cluster_->GetTabletServerFsRoot(0) + "2"; std::string already has a 'using' clause, and string::operator+ is defined for string literal arguments (const char*). 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 check it in the above block? If it's not adding any coverage here it may be easier to just skip adding that. -- 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 18:40:27 +0000 Gerrit-HasComments: Yes
