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 10: (16 comments) http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck-test.cc@337 PS8, Line 337: ASSERT_TRUE(ksck_->FetchInfoFromTabletServers().IsRemoteError()); : ASSERT_STR_CONTAINS(e > Nit: combine and and assert on the particular kind of error. Done http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck-test.cc@339 PS8, Line 339: "Tablet Server Summary\n" > Was this only for debugging? If so, please remove. Done http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck-test.cc@341 PS8, Line 341: "---------+-------------+-------------------\n" > Nit: got tabs here; please convert to spaces. Done http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck.h File src/kudu/tools/ksck.h: http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck.h@482 PS8, Line 482: HEALTHY, > Nit: should use 2-char indentation here, not 4. Basically the same as in Ch Done http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck.cc@193 PS8, Line 193: CHECK_OK(pool->SubmitFunc([&]() { > I think you should use Kudu's simple_spinlock class here instead. The lock Done http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck.cc@215 PS8, Line 215: pool->Wait(); > Use std::lock_guard to acquire/release the lock in an RAII way. Done http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck.cc@272 PS8, Line 272: break; > I'd make this its own statement and do a LOG(FATAL) or something, since if Done http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck_remote-test.cc@52 PS8, Line 52: #include "kudu/util/test_util.h" > Nit: shouldn't this go just after monotime.h to preserve alphabetical sort Done http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck_remote-test.cc@228 PS8, Line 228: TEST_F(RemoteKsckTest, TestTabletServerMismatchUUID) { > Nit: 2 char indentation. Done http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck_remote-test.cc@232 PS8, Line 232: tserver::MiniTabletServer* tablet_server = mini_cluster_->mini_tablet_server(0); > We have "using std::string" on L68 so you needn't qualify strings with the Done http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck_remote-test.cc@238 PS8, Line 238: tserver::MiniTabletServer new_tablet_server(root_dir, address); : ASSERT_OK(new_tablet_server.Start( > These return a Status, right? If so you should ASSERT_OK them. Done http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck_remote-test.cc@248 PS8, Line 248: "($0) doesn't match the expected ID: $1"; > Can we assert on the particular kind of error? Like, ASSERT_TRUE(s.IsRemote Done http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck_remote.cc File src/kudu/tools/ksck_remote.cc: http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck_remote.cc@102 PS8, Line 102: { > Nit: should be 2 char indentation. Done http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck_remote.cc@111 PS8, Line 111: return Status::RemoteError(Substitute("ID reported by tablet server ($0) doesn't " : "match the expected ID: $1", > Nit: these continuation lines should be aligned with the double quote in "I Done http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck_remote.cc@113 PS8, Line 113: response_uuid, uuid())); > Nit: since 'message' is used in just one place, you could embed Substitute( Done http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck_remote.cc@123 PS8, Line 123: Status s = ts_proxy_->ListTablets(req, &resp, &rpc); : RETURN_NOT_OK_PREPEND(ts_proxy_->ListTablets(req, &resp, &rpc), : "could not list tablets"); : if (resp.has_error()) { : > This can be simplified: Done -- 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: 10 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: Tidy Bot Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Wed, 28 Mar 2018 17:17:32 +0000 Gerrit-HasComments: Yes
