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 10: (8 comments) http://gerrit.cloudera.org:8080/#/c/9787/10/src/kudu/tools/ksck.h File src/kudu/tools/ksck.h: http://gerrit.cloudera.org:8080/#/c/9787/10/src/kudu/tools/ksck.h@529 PS10, Line 529: : static Status PrintTabletServerSummaries(const std::vector<TabletServerSummary>& : tablet_server_summaries, : std::ostream& out); Nit: when a declaration gets too long like this, rather than split the type and the variable name (which can be confusing; here it looks like the function takes three parameters and not two), format like this: static Status PrintTabletServerSummaries( const std::vector<TabletServerSummary>& tablet_server_summaries, std::ostream& out); http://gerrit.cloudera.org:8080/#/c/9787/10/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: http://gerrit.cloudera.org:8080/#/c/9787/10/src/kudu/tools/ksck.cc@42 PS10, Line 42: #include "kudu/gutil/spinlock.h" Nit: alphabetical sorting would dictate that this be above gutil/strings/... http://gerrit.cloudera.org:8080/#/c/9787/10/src/kudu/tools/ksck.cc@258 PS10, Line 258: TabletServerSummary Nit: could use 'auto' here instead. More terse. http://gerrit.cloudera.org:8080/#/c/9787/10/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: http://gerrit.cloudera.org:8080/#/c/9787/10/src/kudu/tools/ksck_remote-test.cc@242 PS10, Line 242: std::string new_uuid = new_tablet_server.uuid(); Nit: don't need std:: prefixing here or below. http://gerrit.cloudera.org:8080/#/c/9787/10/src/kudu/tools/ksck_remote-test.cc@245 PS10, Line 245: Status s = ksck_->FetchInfoFromTabletServers(); Don't need this anymore. http://gerrit.cloudera.org:8080/#/c/9787/10/src/kudu/tools/ksck_remote.cc File src/kudu/tools/ksck_remote.cc: http://gerrit.cloudera.org:8080/#/c/9787/10/src/kudu/tools/ksck_remote.cc@107 PS10, Line 107: Status s = generic_proxy_->GetStatus(req, &resp, &rpc); : RETURN_NOT_OK_PREPEND(s, "could not get status from server"); Combine these? No need for 's' anymore. http://gerrit.cloudera.org:8080/#/c/9787/10/src/kudu/tools/ksck_remote.cc@109 PS10, Line 109: std::string response_uuid = resp.status().node_instance().permanent_uuid(); Nit: no std:: prefix. http://gerrit.cloudera.org:8080/#/c/9787/10/src/kudu/tools/ksck_remote.cc@123 PS10, Line 123: Status s = ts_proxy_->ListTablets(req, &resp, &rpc); Don't need this line anymore. -- 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 <abu...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Wed, 28 Mar 2018 17:31:22 +0000 Gerrit-HasComments: Yes
