Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11554 )
Change subject: [tools] KUDU-2179: Have ksck not use a single snapshot for all tablets ...................................................................... Patch Set 7: (14 comments) http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.h File src/kudu/tools/ksck_checksum.h: http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.h@45 PS7, Line 45: } nit: add "// namespace rpc" http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.h@140 PS7, Line 140: ~KsckChecksumManager() = default; Is this needed? http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.h@169 PS7, Line 169: const MonoDelta& timeout, : const MonoDelta& idle_timeout, Why not use the `timeout` and `idle_timeout` in `opts_`? http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.h@211 PS7, Line 211: // Initialize 'ts_open_slots_map_'. : void InitializeTsSlotsMap(); Does it matter whether this is called more than once? If not, maybe name it "PopulateTsSlotsMap" or something? http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.h@256 PS7, Line 256: in nit: on http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.cc File src/kudu/tools/ksck_checksum.cc: http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.cc@86 PS7, Line 86: when the all nit: extra word http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.cc@118 PS7, Line 118: nit: spacing http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.cc@256 PS7, Line 256: int64_t delta_rows, int64_t delta_bytes Let's DCHECK that these are positive. http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.cc@293 PS7, Line 293: MonoTime::Now() nit: reuse `start`? http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.cc@298 PS7, Line 298: int No more `auto`? http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.cc@304 PS7, Line 304: nit: spacing http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.cc@341 PS7, Line 341: auto& slots_open = FindOrDie(ts_slots_open_map_, replica->ts_uuid()); nit: perhaps put these into a vector so you don't have to re-FindOrDie them? http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.cc@402 PS7, Line 402: VLOG(1) << LogPrefix(tablet_info.tablet->id()) << "Using timestamp " : << timestamp_for_tablet; What should happen if `timestamp_for_tablet` is kCurrentTimestamp? I know we checked before creating the manager that there were healthy tservers, but that could change as the scans progress, right? http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.cc@620 PS7, Line 620: tablet_infos, : tablet_servers, : &manager)); nit: spacing -- To view, visit http://gerrit.cloudera.org:8080/11554 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iff0905c2099e6f56ed1cb651611918acbaf75476 Gerrit-Change-Number: 11554 Gerrit-PatchSet: 7 Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Wed, 03 Oct 2018 20:13:41 +0000 Gerrit-HasComments: Yes
