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/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11554/9//COMMIT_MSG@15 PS9, Line 15: before > nit: before --> behind ? 'before' is ok because we're talking about (something sort of like) time. http://gerrit.cloudera.org:8080/#/c/11554/9//COMMIT_MSG@60 PS9, Line 60: y large c > BTW, what happens if _some_ of the peers are unavailable? Do we need to ad It's not special. http://gerrit.cloudera.org:8080/#/c/11554/9/src/kudu/tools/ksck.h File src/kudu/tools/ksck.h: http://gerrit.cloudera.org:8080/#/c/11554/9/src/kudu/tools/ksck.h@353 PS9, Line 353: timestamp_ = timestamp; : } : > Is it used at all? Done http://gerrit.cloudera.org:8080/#/c/11554/9/src/kudu/tools/ksck_checksum.h File src/kudu/tools/ksck_checksum.h: http://gerrit.cloudera.org:8080/#/c/11554/9/src/kudu/tools/ksck_checksum.h@256 PS9, Line 256: // be checksummed based in available slots on tablet servers. : gscoped_ptr<ThreadPool> find_tablets_t > nit: why not to move into std::atomic with those as well? 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@358 PS7, Line 358: shared_ptr<rpc::Messenger> messenger; : rpc::MessengerBuilder builder("timestamp update"); : RETURN_NOT_OK(builder.Build(&messenger)); > I never really saw the point of the class hierarchy here; we could have pok Done http://gerrit.cloudera.org:8080/#/c/11554/9/src/kudu/tools/ksck_checksum.cc File src/kudu/tools/ksck_checksum.cc: http://gerrit.cloudera.org:8080/#/c/11554/9/src/kudu/tools/ksck_checksum.cc@66 PS9, Line 66: DEFINE_int32(max_progress_report_wait_ms, 5000, : "Maximum number of milliseconds to wait between progress reports. " : "Used to speed up tests."); : TAG_FLAG(max_progress_report_wait_ms, hidden); > Could this be also useful while using the tool in the field? I don't want to expose it because it's meant for tests. The updates are very short, there's really no reason to get them more or less often, and it just pollutes the option list with one nobody is really gonna use. http://gerrit.cloudera.org:8080/#/c/11554/9/src/kudu/tools/ksck_checksum.cc@70 PS9, Line 70: timestamp_update_period_ms > Does it make sense to keep it in milliseconds? Maybe, a second-grain resol I guess, but I don't even think it's worth changing. http://gerrit.cloudera.org:8080/#/c/11554/9/src/kudu/tools/ksck_checksum.cc@86 PS9, Line 86: when the all the checksums for its replicas b > nit: maybe, change to 'when the checksumming for all its replicas begins' I don't think there's a meaningful difference there. http://gerrit.cloudera.org:8080/#/c/11554/9/src/kudu/tools/ksck_checksum.cc@101 PS9, Line 101: listed in 'tservers' > What about the reverse: is it OK to have a tablet server in 'tservers' that Yes. It could be one with no replicas on it. http://gerrit.cloudera.org:8080/#/c/11554/9/src/kudu/tools/ksck_checksum.cc@106 PS9, Line 106: CHECK(num_replicas); > nit: maybe, DCHECK is enough, since it would crash anyway further down if i I think the crash is cleaner if it's a CHECK failure. This isn't a hot path, anyway. http://gerrit.cloudera.org:8080/#/c/11554/9/src/kudu/tools/ksck_checksum.cc@110 PS9, Line 110: tserver_uuid_set > Consider using helper functions from map-util.h: those present more express Done http://gerrit.cloudera.org:8080/#/c/11554/9/src/kudu/tools/ksck_checksum.cc@187 PS9, Line 187: bool KsckChecksumManager::HasOpenTsSlotsUnlocked() { > nit: add const specifier? Done http://gerrit.cloudera.org:8080/#/c/11554/9/src/kudu/tools/ksck_checksum.cc@383 PS9, Line 383: r (const auto& ts : > nit: use helper functions from map-util.h Done http://gerrit.cloudera.org:8080/#/c/11554/9/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: http://gerrit.cloudera.org:8080/#/c/11554/9/src/kudu/tools/ksck_remote-test.cc@429 PS9, Line 429: or > and Done http://gerrit.cloudera.org:8080/#/c/11554/9/src/kudu/tools/ksck_remote-test.cc@428 PS9, Line 428: #if defined(THREAD_SANITIZER) || defined(ADDRESS_SANITIZER) : LOG(WARNING) << "test is skipped in TSAN or ASAN builds"; : return; : #endif > nit: shift this left one indent Why? That looks really funny. http://gerrit.cloudera.org:8080/#/c/11554/9/src/kudu/tools/ksck_remote-test.cc@476 PS9, Line 476: MonoDelta::FromSeconds(timeout_sec), : MonoDelta::FromSeconds(timeout_sec), : scan_concurrency, : use_snapshot, : KsckChecksumOptions::kCurrentTimestamp))); > nit: indent should be 4 spaces from the outer scope's indent. Done http://gerrit.cloudera.org:8080/#/c/11554/9/src/kudu/tools/ksck_remote.h File src/kudu/tools/ksck_remote.h: http://gerrit.cloudera.org:8080/#/c/11554/9/src/kudu/tools/ksck_remote.h@130 PS9, Line 130: > style nit: indent should be 4 spaces from the outer scope Done http://gerrit.cloudera.org:8080/#/c/11554/9/src/kudu/tools/ksck_remote.cc File src/kudu/tools/ksck_remote.cc: http://gerrit.cloudera.org:8080/#/c/11554/9/src/kudu/tools/ksck_remote.cc@321 PS9, Line 321: const Schema& schema, > nit: maybe, use the move semantics while passing this parameter as well? I should, but there's no move constructor or assignment for Schema. I made a comment on an earlier version that I should implement it. -- 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 20:11:47 +0000 Gerrit-HasComments: Yes
