You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Will Berkeley (Code Review)" <ge...@cloudera.org> on 2018/09/24 09:33:58 UTC
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11498
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
[tools] ksck checksums: Add KsckChecksummer class
This removes the remaining checksum logic out of Ksck and into a new
KsckChecksummer class, along with refactoring some parts of the logic
into separate functions. There are no functional changes.
The logic that was not refactored is directly related to KUDU-2179, and
will be addressed in a follow-up patch.
Change-Id: I2016936eaa26fd6b499783e7d5d8f404816b37fa
---
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_checksum.cc
M src/kudu/tools/ksck_checksum.h
4 files changed, 333 insertions(+), 184 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/11498/1
--
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: newchange
Gerrit-Change-Id: I2016936eaa26fd6b499783e7d5d8f404816b37fa
Gerrit-Change-Number: 11498
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11498 )
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
Patch Set 8: Code-Review+2
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11498/8/src/kudu/tools/ksck_checksum.h
File src/kudu/tools/ksck_checksum.h:
http://gerrit.cloudera.org:8080/#/c/11498/8/src/kudu/tools/ksck_checksum.h@176
PS8, Line 176: ~TabletServerChecksumCallbacks() = default;
nit: it would be nice to add a comment like the following:
// Prevent instances of this class to be allocated on the stack.
http://gerrit.cloudera.org:8080/#/c/11498/6/src/kudu/tools/ksck_checksum.cc
File src/kudu/tools/ksck_checksum.cc:
http://gerrit.cloudera.org:8080/#/c/11498/6/src/kudu/tools/ksck_checksum.cc@372
PS6, Line 372: if (queue->BlockingGet(&table_tablet))
> That means the queue is empty (since it won't block because it was shut dow
Right, so my question was whether it makes sense to continue the outer for() cycle in that case or just break out of it. Probably, that doesn't matter.
http://gerrit.cloudera.org:8080/#/c/11498/8/src/kudu/tools/ksck_checksum.cc
File src/kudu/tools/ksck_checksum.cc:
http://gerrit.cloudera.org:8080/#/c/11498/8/src/kudu/tools/ksck_checksum.cc@205
PS8, Line 205:
nit: this extra space is not needed
--
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: 8
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 02 Oct 2018 23:51:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11498
to look at the new patch set (#2).
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
[tools] ksck checksums: Add KsckChecksummer class
This removes the remaining checksum logic out of Ksck and into a new
KsckChecksummer class, along with refactoring some parts of the logic
into separate functions. There are no functional changes.
The logic that was not refactored is directly related to KUDU-2179, and
will be addressed in a follow-up patch.
Change-Id: I2016936eaa26fd6b499783e7d5d8f404816b37fa
---
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_checksum.cc
M src/kudu/tools/ksck_checksum.h
4 files changed, 343 insertions(+), 188 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/11498/2
--
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: newpatchset
Gerrit-Change-Id: I2016936eaa26fd6b499783e7d5d8f404816b37fa
Gerrit-Change-Number: 11498
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11498 )
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
Patch Set 6: Code-Review+1
(1 comment)
I'm planning to take another look tomorrow morning since I'm a bit sleepy right now, but so far looks good.
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@186
PS3, Line 186: public:
> Thanks. Sorry to blow off some of these nits, it's just painful to rebase o
That's totally fine with me, np.
--
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: 6
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 02 Oct 2018 06:29:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11498 )
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
Patch Set 9: Code-Review+2
--
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: 9
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 03 Oct 2018 18:02:40 +0000
Gerrit-HasComments: No
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11498
to look at the new patch set (#5).
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
[tools] ksck checksums: Add KsckChecksummer class
This removes the remaining checksum logic out of Ksck and into a new
KsckChecksummer class, along with refactoring some parts of the logic
into separate functions. There are no functional changes.
The logic that was not refactored is directly related to KUDU-2179, and
will be addressed in a follow-up patch.
Change-Id: I2016936eaa26fd6b499783e7d5d8f404816b37fa
---
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_checksum.cc
M src/kudu/tools/ksck_checksum.h
4 files changed, 335 insertions(+), 188 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/11498/5
--
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: newpatchset
Gerrit-Change-Id: I2016936eaa26fd6b499783e7d5d8f404816b37fa
Gerrit-Change-Number: 11498
Gerrit-PatchSet: 5
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11498 )
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/11498/1/src/kudu/tools/ksck_checksum.h
File src/kudu/tools/ksck_checksum.h:
http://gerrit.cloudera.org:8080/#/c/11498/1/src/kudu/tools/ksck_checksum.h@193
PS1, Line 193: Status ChecksumData(const KsckChecksumOptions& opts,
> warning: function 'kudu::tools::KsckChecksummer::ChecksumData' has a defini
Done
http://gerrit.cloudera.org:8080/#/c/11498/1/src/kudu/tools/ksck_checksum.cc
File src/kudu/tools/ksck_checksum.cc:
http://gerrit.cloudera.org:8080/#/c/11498/1/src/kudu/tools/ksck_checksum.cc@87
PS1, Line 87: const vector<string>& table_filters,
> warning: pass by value and use std::move [modernize-pass-by-value]
Done
http://gerrit.cloudera.org:8080/#/c/11498/1/src/kudu/tools/ksck_checksum.cc@88
PS1, Line 88: const vector<string>& tablet_id_filters)
> warning: pass by value and use std::move [modernize-pass-by-value]
Done
http://gerrit.cloudera.org:8080/#/c/11498/1/src/kudu/tools/ksck_checksum.cc@261
PS1, Line 261: shared_ptr<KsckTabletServer> ts = FindOrDie(cluster.tablet_servers(),
> warning: the variable 'ts' is copy-constructed from a const reference but i
Done
http://gerrit.cloudera.org:8080/#/c/11498/1/src/kudu/tools/ksck_checksum.cc@282
PS1, Line 282: std::move(replica_checksum));
> warning: passing result of std::move() as a const reference argument; no mo
Done
http://gerrit.cloudera.org:8080/#/c/11498/1/src/kudu/tools/ksck_checksum.cc@286
PS1, Line 286: std::move(tablet_checksum));
> warning: passing result of std::move() as a const reference argument; no mo
Done
http://gerrit.cloudera.org:8080/#/c/11498/1/src/kudu/tools/ksck_checksum.cc@289
PS1, Line 289: InsertOrDie(table_checksum_map, table->name(), std::move(table_checksum));
> warning: passing result of std::move() as a const reference argument; no mo
Done
--
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: 1
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 24 Sep 2018 15:15:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 02 Oct 2018 06:02:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11498 )
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
Patch Set 2:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:
http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck.cc@a637
PS2, Line 637:
> nit: Maybe keep this around as a note? It's probably not too difficult to r
Done
http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck.cc@425
PS2, Line 425: table_filters_for_checksum_opts,
: tablet_id_filters_for_checksum_opts
> We should std::move these
Done
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();
:
: 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);
> Did you consider adding KsckCheckOptionsBuilder instead?
No, because this isn't a client-facing API, and these 4 cover all the actual cases.
http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.h@186
PS3, Line 186: class KsckChecksummer {
> nit: do you mind re-order the methods in the ksck_checksum.cc file to match
The method changes a fair amount in a follow-up, enough so that a rebase is tedious and error-prone. Is it ok to leave ChecksumData out of place for this patch and move it in the follow-up?
http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.h@188
PS3, Line 188: explicit KsckChecksummer(KsckCluster* cluster);
> docs nit: does it make sense to add a note explaining that 'cluster' should
Done
http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.h@216
PS3, Line 216: // NOTE: Even if this function returns a bad Status, 'table_checksum_map'
: // and 'num_results' will still be populated using whatever results are
: // available.
> Is that implementation detail is exposed because you want somebody to rely
It's not an implementation detail. It's an important part of how the class works: even if we checksum 10/20 replicas before we run out of time, the 10 results should still be reported. This comment is just to make this extra clear.
http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck_checksum.cc
File src/kudu/tools/ksck_checksum.cc:
http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck_checksum.cc@188
PS2, Line 188: const KsckCluster& cluster,
> `cluster_` is already a private member. What's the thought process behind m
I feel like making the method static means how it affects state is more explicit, while keeping it tied to the class it's relevant to. Making it static says both "this doesn't affect the state of an instance" and "this doesn't involve the state of the instance". Instead, it's just a related function, like these two which just do some data shuffling and populating into an out param.
I'm not married to the concept though. I can make them const methods if you like.
http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck_checksum.cc@345
PS2, Line 345: timestamp of the
: // some
> nit: extra word
Done
http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.cc
File src/kudu/tools/ksck_checksum.cc:
http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.cc@186
PS3, Line 186: St
> spacing nit: this supposed to be 4 spaces
Done
http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.cc@186
PS3, Line 186: us KsckChecksummer::BuildTabletTableMap(
: const KsckChe
> nit: it might be
Done
http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.cc@267
PS3, Line 267:
> nit: would 'auto' work here as well?
Done
--
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: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 01 Oct 2018 21:54:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11498 )
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
Patch Set 6: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11498/6/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:
http://gerrit.cloudera.org:8080/#/c/11498/6/src/kudu/tools/ksck.cc@538
PS6, Line 538: ad hob pr
nit: ad hoc
--
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: 6
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 02 Oct 2018 14:44:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11498 )
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11498/4/src/kudu/tools/ksck_checksum.h
File src/kudu/tools/ksck_checksum.h:
http://gerrit.cloudera.org:8080/#/c/11498/4/src/kudu/tools/ksck_checksum.h@189
PS4, Line 189: ~KsckChecksummer() = default;
Is this needed?
http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck_checksum.cc
File src/kudu/tools/ksck_checksum.cc:
http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck_checksum.cc@188
PS2, Line 188: : cluster_(CHECK_NOTNULL(cl
> I feel like making the method static means how it affects state is more exp
I think it makes sense to be const: it doesn't affect the state of an instance. But in this case, it _does_ involve the state of the instance via `cluster_`, doesn't it? And to take it a step farther, `opts` could be a const member of the KsckChecksummer too, rather than passing (supposedly) the same `opts` to each method.
--
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: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 01 Oct 2018 22:07:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11498 )
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
Patch Set 3:
(7 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();
:
: 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);
Did you consider adding KsckCheckOptionsBuilder instead?
http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.h@186
PS3, Line 186: class KsckChecksummer {
nit: do you mind re-order the methods in the ksck_checksum.cc file to match their declaration order?
http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.h@188
PS3, Line 188: explicit KsckChecksummer(KsckCluster* cluster);
docs nit: does it make sense to add a note explaining that 'cluster' should be alive for the whole lifetime of an instance of this class?
http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.h@216
PS3, Line 216: // NOTE: Even if this function returns a bad Status, 'table_checksum_map'
: // and 'num_results' will still be populated using whatever results are
: // available.
Is that implementation detail is exposed because you want somebody to rely on this or that's more about implementation optimization?
http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.cc
File src/kudu/tools/ksck_checksum.cc:
http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.cc@186
PS3, Line 186:
spacing nit: this supposed to be 4 spaces
http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.cc@186
PS3, Line 186: cluster_(cluster) {
: CHECK(cluster);
nit: it might be
: cluster_(CHECK_NOTNULL(cluster)) {
}
http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.cc@267
PS3, Line 267: string
nit: would 'auto' work here as well?
--
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: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 01 Oct 2018 21:25:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11498 )
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:
http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck.cc@a637
PS2, Line 637:
nit: Maybe keep this around as a note? It's probably not too difficult to reason about why we don't want to print progress updates if we're printing JSON, but I think calling it out explicitly is still helpful.
http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck.cc@425
PS2, Line 425: table_filters_for_checksum_opts,
: tablet_id_filters_for_checksum_opts
We should std::move these
http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck_checksum.cc
File src/kudu/tools/ksck_checksum.cc:
http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck_checksum.cc@188
PS2, Line 188: const KsckCluster& cluster,
`cluster_` is already a private member. What's the thought process behind making `BuildTabletTableMap()` and `CollateChecksumResults()` static?
http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck_checksum.cc@345
PS2, Line 345: timestamp of the
: // some
nit: extra word
--
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: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 01 Oct 2018 15:33:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11498 )
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
Patch Set 9: Code-Review+2
--
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: 9
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 03 Oct 2018 17:30:05 +0000
Gerrit-HasComments: No
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11498 )
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
Patch Set 8:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11498/8/src/kudu/tools/ksck_checksum.h
File src/kudu/tools/ksck_checksum.h:
http://gerrit.cloudera.org:8080/#/c/11498/8/src/kudu/tools/ksck_checksum.h@176
PS8, Line 176: ~TabletServerChecksumCallbacks() = default;
> nit: it would be nice to add a comment like the following:
True, but this class gets deleted in the follow-up :)
http://gerrit.cloudera.org:8080/#/c/11498/6/src/kudu/tools/ksck_checksum.cc
File src/kudu/tools/ksck_checksum.cc:
http://gerrit.cloudera.org:8080/#/c/11498/6/src/kudu/tools/ksck_checksum.cc@372
PS6, Line 372: if (queue->BlockingGet(&table_tablet))
> Right, so my question was whether it makes sense to continue the outer for(
Yeah, it doesn't matter at all because this part changes completely in the follow-up :)
http://gerrit.cloudera.org:8080/#/c/11498/8/src/kudu/tools/ksck_checksum.cc
File src/kudu/tools/ksck_checksum.cc:
http://gerrit.cloudera.org:8080/#/c/11498/8/src/kudu/tools/ksck_checksum.cc@205
PS8, Line 205:
> nit: this extra space is not needed
Done
--
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: 8
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 03 Oct 2018 05:11:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11498 )
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
[tools] ksck checksums: Add KsckChecksummer class
This removes the remaining checksum logic out of Ksck and into a new
KsckChecksummer class, along with refactoring some parts of the logic
into separate functions. There are no functional changes.
The logic that was not refactored is directly related to KUDU-2179, and
will be addressed in a follow-up patch.
Change-Id: I2016936eaa26fd6b499783e7d5d8f404816b37fa
Reviewed-on: http://gerrit.cloudera.org:8080/11498
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_checksum.cc
M src/kudu/tools/ksck_checksum.h
4 files changed, 337 insertions(+), 188 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
Andrew Wong: Looks good to me, approved
--
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: merged
Gerrit-Change-Id: I2016936eaa26fd6b499783e7d5d8f404816b37fa
Gerrit-Change-Number: 11498
Gerrit-PatchSet: 10
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11498
to look at the new patch set (#7).
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
[tools] ksck checksums: Add KsckChecksummer class
This removes the remaining checksum logic out of Ksck and into a new
KsckChecksummer class, along with refactoring some parts of the logic
into separate functions. There are no functional changes.
The logic that was not refactored is directly related to KUDU-2179, and
will be addressed in a follow-up patch.
Change-Id: I2016936eaa26fd6b499783e7d5d8f404816b37fa
---
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_checksum.cc
M src/kudu/tools/ksck_checksum.h
4 files changed, 335 insertions(+), 188 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/11498/7
--
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: newpatchset
Gerrit-Change-Id: I2016936eaa26fd6b499783e7d5d8f404816b37fa
Gerrit-Change-Number: 11498
Gerrit-PatchSet: 7
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11498 )
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11498/6/src/kudu/tools/ksck_checksum.cc
File src/kudu/tools/ksck_checksum.cc:
http://gerrit.cloudera.org:8080/#/c/11498/6/src/kudu/tools/ksck_checksum.cc@183
PS6, Line 183: delete this;
pedantic nit: do you mind adding a private destructor into TabletServerChecksumCallbacks class to make sure it's not instantiated on the stack?
--
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: 6
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 02 Oct 2018 20:34:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11498
to look at the new patch set (#9).
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
[tools] ksck checksums: Add KsckChecksummer class
This removes the remaining checksum logic out of Ksck and into a new
KsckChecksummer class, along with refactoring some parts of the logic
into separate functions. There are no functional changes.
The logic that was not refactored is directly related to KUDU-2179, and
will be addressed in a follow-up patch.
Change-Id: I2016936eaa26fd6b499783e7d5d8f404816b37fa
---
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_checksum.cc
M src/kudu/tools/ksck_checksum.h
4 files changed, 337 insertions(+), 188 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/11498/9
--
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: newpatchset
Gerrit-Change-Id: I2016936eaa26fd6b499783e7d5d8f404816b37fa
Gerrit-Change-Number: 11498
Gerrit-PatchSet: 9
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11498
to look at the new patch set (#8).
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
[tools] ksck checksums: Add KsckChecksummer class
This removes the remaining checksum logic out of Ksck and into a new
KsckChecksummer class, along with refactoring some parts of the logic
into separate functions. There are no functional changes.
The logic that was not refactored is directly related to KUDU-2179, and
will be addressed in a follow-up patch.
Change-Id: I2016936eaa26fd6b499783e7d5d8f404816b37fa
---
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_checksum.cc
M src/kudu/tools/ksck_checksum.h
4 files changed, 337 insertions(+), 188 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/11498/8
--
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: newpatchset
Gerrit-Change-Id: I2016936eaa26fd6b499783e7d5d8f404816b37fa
Gerrit-Change-Number: 11498
Gerrit-PatchSet: 8
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11498 )
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
Patch Set 6:
(1 comment)
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@186
PS3, Line 186: public:
> I think it's OK to leave it as is for now: my comment was just a pedantic n
Thanks. Sorry to blow off some of these nits, it's just painful to rebase on them because the KUDU-2179 changes are pretty significant.
--
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: 6
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 02 Oct 2018 06:23:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11498
to look at the new patch set (#3).
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
[tools] ksck checksums: Add KsckChecksummer class
This removes the remaining checksum logic out of Ksck and into a new
KsckChecksummer class, along with refactoring some parts of the logic
into separate functions. There are no functional changes.
The logic that was not refactored is directly related to KUDU-2179, and
will be addressed in a follow-up patch.
Change-Id: I2016936eaa26fd6b499783e7d5d8f404816b37fa
---
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_checksum.cc
M src/kudu/tools/ksck_checksum.h
4 files changed, 341 insertions(+), 188 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/11498/3
--
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: newpatchset
Gerrit-Change-Id: I2016936eaa26fd6b499783e7d5d8f404816b37fa
Gerrit-Change-Number: 11498
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11498
to look at the new patch set (#4).
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
[tools] ksck checksums: Add KsckChecksummer class
This removes the remaining checksum logic out of Ksck and into a new
KsckChecksummer class, along with refactoring some parts of the logic
into separate functions. There are no functional changes.
The logic that was not refactored is directly related to KUDU-2179, and
will be addressed in a follow-up patch.
Change-Id: I2016936eaa26fd6b499783e7d5d8f404816b37fa
---
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_checksum.cc
M src/kudu/tools/ksck_checksum.h
4 files changed, 341 insertions(+), 188 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/11498/4
--
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: newpatchset
Gerrit-Change-Id: I2016936eaa26fd6b499783e7d5d8f404816b37fa
Gerrit-Change-Number: 11498
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11498 )
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11498/4/src/kudu/tools/ksck_checksum.h
File src/kudu/tools/ksck_checksum.h:
http://gerrit.cloudera.org:8080/#/c/11498/4/src/kudu/tools/ksck_checksum.h@189
PS4, Line 189: ~KsckChecksummer() = default;
> Is this needed?
Done
http://gerrit.cloudera.org:8080/#/c/11498/4/src/kudu/tools/ksck_checksum.h@189
PS4, Line 189: ~KsckChecksummer() = default;
> Is this needed?
Done
http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck_checksum.cc
File src/kudu/tools/ksck_checksum.cc:
http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck_checksum.cc@188
PS2, Line 188: : cluster_(CHECK_NOTNULL(cl
> I think it makes sense to be const: it doesn't affect the state of an insta
Done
--
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: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 01 Oct 2018 22:59:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11498 )
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11498/6/src/kudu/tools/ksck_checksum.cc
File src/kudu/tools/ksck_checksum.cc:
http://gerrit.cloudera.org:8080/#/c/11498/6/src/kudu/tools/ksck_checksum.cc@205
PS6, Line 205: num_tablets += table->tablets().size();
I'm curious why the number of tablets is incremented regardless of the tablet_id filter. If there is a tablet_id filter which does not match any tablet (but table_name filter matches), the code below will issue a warning about not finding any replicas, as if a table where every partition has been dropped.
Is that intended behavior?
http://gerrit.cloudera.org:8080/#/c/11498/6/src/kudu/tools/ksck_checksum.cc@227
PS6, Line 227:
nit: for better readability, maybe drop this space but prefix 'table_filters=...' and 'tablet_id_filters=...' messages below with spaces.
http://gerrit.cloudera.org:8080/#/c/11498/6/src/kudu/tools/ksck_checksum.cc@372
PS6, Line 372: if (queue->BlockingGet(&table_tablet))
What if BlockingGet() returns false? Does it make sense to continue in that case?
http://gerrit.cloudera.org:8080/#/c/11498/6/src/kudu/tools/ksck_checksum.cc@388
PS6, Line 388: Status s = CollateChecksumResults(reporter->checksums(),
nit: consider adding 'const' specifier -- the code goes further and it's easier to read when you know it's not going to change.
--
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: 6
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 02 Oct 2018 19:57:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck checksums: Add KsckChecksummer class
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11498 )
Change subject: [tools] ksck checksums: Add KsckChecksummer class
......................................................................
Patch Set 6:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/11498/6/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:
http://gerrit.cloudera.org:8080/#/c/11498/6/src/kudu/tools/ksck.cc@538
PS6, Line 538: ad hob pr
> nit: ad hoc
Done
http://gerrit.cloudera.org:8080/#/c/11498/6/src/kudu/tools/ksck_checksum.cc
File src/kudu/tools/ksck_checksum.cc:
http://gerrit.cloudera.org:8080/#/c/11498/6/src/kudu/tools/ksck_checksum.cc@183
PS6, Line 183: delete this;
> pedantic nit: do you mind adding a private destructor into TabletServerChec
Done
http://gerrit.cloudera.org:8080/#/c/11498/6/src/kudu/tools/ksck_checksum.cc@205
PS6, Line 205: num_tablets += table->tablets().size();
> I'm curious why the number of tablets is incremented regardless of the tabl
Yeah, I think so.
http://gerrit.cloudera.org:8080/#/c/11498/6/src/kudu/tools/ksck_checksum.cc@227
PS6, Line 227:
> nit: for better readability, maybe drop this space but prefix 'table_filter
Done
http://gerrit.cloudera.org:8080/#/c/11498/6/src/kudu/tools/ksck_checksum.cc@372
PS6, Line 372: if (queue->BlockingGet(&table_tablet))
> What if BlockingGet() returns false? Does it make sense to continue in tha
That means the queue is empty (since it won't block because it was shut down).
http://gerrit.cloudera.org:8080/#/c/11498/6/src/kudu/tools/ksck_checksum.cc@388
PS6, Line 388: Status s = CollateChecksumResults(reporter->checksums(),
> nit: consider adding 'const' specifier -- the code goes further and it's ea
Done
--
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: 6
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 02 Oct 2018 21:57:34 +0000
Gerrit-HasComments: Yes