Adar Dembo 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 9: (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: Status s = ksck_->FetchInfoFromTabletServers(); : ASSERT_TRUE(!s.ok()); Nit: combine and and assert on the particular kind of error. http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck-test.cc@339 PS8, Line 339: LOG(INFO) << err_stream_.str(); Was this only for debugging? If so, please remove. http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck-test.cc@341 PS8, Line 341: "Tablet Server Summary\n" Nit: got tabs here; please convert to spaces. 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: // The tablet server is healthy Nit: should use 2-char indentation here, not 4. Basically the same as in CheckResult just above. 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: int i = 0; I think you should use Kudu's simple_spinlock class here instead. The lock is acquired for a very short amount of time; in the worst case, it's held for a memory allocation as the array grows. So a sleeping mutex isn't necessary. http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck.cc@215 PS8, Line 215: tablet_server_summaries_lock.unlock(); Use std::lock_guard to acquire/release the lock in an RAII way. 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 it fires it's a programming error: a sign that other code in ksck.cc needs to be updated. 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/net/net_util.h" Nit: shouldn't this go just after monotime.h to preserve alphabetical sort order? http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck_remote-test.cc@228 PS8, Line 228: ASSERT_OK(ksck_->CheckMasterRunning()); Nit: 2 char indentation. http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck_remote-test.cc@232 PS8, Line 232: std::string old_uuid = tablet_server->uuid(); We have "using std::string" on L68 so you needn't qualify strings with the std:: prefix. http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck_remote-test.cc@238 PS8, Line 238: new_tablet_server.Start(); : new_tablet_server.WaitStarted(); These return a Status, right? If so you should ASSERT_OK them. http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck_remote-test.cc@248 PS8, Line 248: ASSERT_TRUE(!s.ok()); Can we assert on the particular kind of error? Like, ASSERT_TRUE(s.IsRemoteError()) or some such? Plus, since 's' is only used here, you could combine the ASSERT_TRUE into the FetchInfoFromTabletServers() call. 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: server::GetStatusRequestPB req; Nit: should be 2 char indentation. http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck_remote.cc@111 PS8, Line 111: "match the expected ID: $1", : response_uuid, uuid()); Nit: these continuation lines should be aligned with the double quote in "ID reported...". http://gerrit.cloudera.org:8080/#/c/9787/8/src/kudu/tools/ksck_remote.cc@113 PS8, Line 113: return Status::RemoteError(message); Nit: since 'message' is used in just one place, you could embed Substitute() inside Status::RemoteError(...) and avoid declaring 'message' at all. 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(s, "could not list tablets"); : if (s.ok() && resp.has_error()) { : RETURN_NOT_OK(StatusFromPB(resp.error().status())); : } This can be simplified: RETURN_NOT_OK_PREPEND(ts_proxy_->ListTablets(req, &resp, &rpc), "could not list tablets"); if (resp.has_error()) { return StatusFromPB(resp.error().status()) } Basically: 1. RETURN_NOT_OK_PREPEND(ListTablets(...)) is guaranteed to return out if ListTablets() returns !Status::OK, which means there's no need to check 's' after ListTablets(). 2. StatusFromPB() is guaranteed to return a non-OK status because resp.error() is only set if resp.error() isn't OK. -- 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: 9 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 16:31:43 +0000 Gerrit-HasComments: Yes
