Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11498 )

Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.h
File src/kudu/tools/ksck_checksum.h:

http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.h@57
PS3, Line 57:
            :   KsckChecksumOptions(std::vector<std::string> table_filters,
            :                       std::vector<std::string> tablet_id_filters);
            :
            :   KsckChecksumOptions(MonoDelta timeout,
            :                       int scan_concurrency,
            :                       bool use_snapshot,
            :                       uint64_t snapshot_timestamp);
            :
            :   KsckChecksumOptions(MonoDelta timeout,
            :                       int scan_concurrency,
            :                       bool use_snapshot,
            :                       uint64_t snapshot_timestamp,
            :                       std::vector<std::string> table_filters,
            :                       std::vector<std::string> tablet_id_filters);
            :
> No, because this isn't a client-facing API, and these 4 cover all the actua
OK, this looks good enough for the internal consumption.


http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.h@186
PS3, Line 186:  public:
> The method changes a fair amount in a follow-up, enough so that a rebase is
I think it's OK to leave it as is for now: my comment was just a pedantic nit, 
feel free to ignore altogether.  Maybe, another clean-up of this code could be 
done a bit later, if needed at all.


http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.h@216
PS3, Line 216:   Status CollateChecksumResults(
             :       const ChecksumResultReporter::TabletResultMap& checksums,
             :       KsckTable
> It's not an implementation detail. It's an important part of how the class
Great, thank you for the explanation.



--
To view, visit http://gerrit.cloudera.org:8080/11498
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2016936eaa26fd6b499783e7d5d8f404816b37fa
Gerrit-Change-Number: 11498
Gerrit-PatchSet: 5
Gerrit-Owner: Will Berkeley <[email protected]>
Gerrit-Reviewer: Alexey Serbin <[email protected]>
Gerrit-Reviewer: Andrew Wong <[email protected]>
Gerrit-Reviewer: Attila Bukor <[email protected]>
Gerrit-Reviewer: Grant Henke <[email protected]>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <[email protected]>
Gerrit-Reviewer: Will Berkeley <[email protected]>
Gerrit-Comment-Date: Tue, 02 Oct 2018 06:02:01 +0000
Gerrit-HasComments: Yes

Reply via email to