Will Berkeley 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: (18 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" Done http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.h@136 PS7, Line 136: static Status New(KsckChecksumOptions opts, > warning: function 'kudu::tools::KsckChecksumManager::New' has a definition Done http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.h@140 PS7, Line 140: ~KsckChecksumManager() = default; > Is this needed? Done 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_`? Done 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 It shouldn't be called more than once since it sets the number of slots to their initial values and other methods manage them from there. http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.h@256 PS7, Line 256: in > nit: on Done 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 Done http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.cc@118 PS7, Line 118: > nit: spacing Done 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. Done http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.cc@270 PS7, Line 270: auto& tablet_result = LookupOrInsert(&checksums_, > We've got emplace-based functions in map-util.h; you should prefer them to Done http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.cc@279 PS7, Line 279: boost::bind(&KsckChecksumManager::StartTabletChecksums, this)), > Prefer std::bind; I think std::function can implicitly cast to boost::funct Done http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.cc@293 PS7, Line 293: MonoTime::Now() > nit: reuse `start`? Done http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.cc@298 PS7, Line 298: int > No more `auto`? If I do that I need to cast 'FLAGS_...' to match whatever is inferred for 'rem_ms', or cast 'rem_ms' to be an int32_t, removing the point of auto. I'll change it to the former alternative and you can see. http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.cc@304 PS7, Line 304: > nit: spacing Done 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 Done http://gerrit.cloudera.org:8080/#/c/11554/7/src/kudu/tools/ksck_checksum.cc@358 PS7, Line 358: shared_ptr<rpc::Messenger> messenger; : rpc::MessengerBuilder builder("timestamp update"); : RETURN_NOT_OK(builder.Build(&messenger)); > Can't you reuse the Messenger that ksck uses for RPCs? There is no Messenger in a KsckCluster, the base class this class works with. There's only a Messenger in a RemoteKsckCluster, and it's held privately. I guess I can add a virtual KsckCluster::messenger() getter that returns a shared_ptr to a KsckCluster's Messenger, which just constructs a new Messenger when called on a MockKsckCluster? That seems a little shady. Does it sound worth it to you? 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 w In that case we probably ought to short-circuit the checksum with an error. 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 Done -- 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: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Thu, 04 Oct 2018 06:04:22 +0000 Gerrit-HasComments: Yes
