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/04/01 07:41:03 UTC
[kudu-CR] [tools] ksck improvements [1/n]: Eliminate KsckMaster class
Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9881
Change subject: [tools] ksck improvements [1/n]: Eliminate KsckMaster class
......................................................................
[tools] ksck improvements [1/n]: Eliminate KsckMaster class
ksck has a KsckCluster and a KsckMaster class. The KsckCluster class's
methods more-or-less map one-to-one with KsckMaster class methods.
The KsckMaster class represents the collective masters of a cluster,
not an individual master.
This patch combines the two classes into a KsckCluster class. This
class is the base class for a MockKsckCluster class and a
RemoteKsckCluster class, for tests and "real life", respectively.
This is just refactoring. There are no functional changes in this patch.
Change-Id: I21f9e244b6ba10e11327c744abc1ac72a9b2ab1c
---
M src/kudu/client/schema.h
M src/kudu/integration-tests/cluster_verifier.cc
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_tablet.cc
10 files changed, 116 insertions(+), 179 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/9881/1
--
To view, visit http://gerrit.cloudera.org:8080/9881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I21f9e244b6ba10e11327c744abc1ac72a9b2ab1c
Gerrit-Change-Number: 9881
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] ksck improvements [1/n]: Eliminate KsckMaster class
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9881 )
Change subject: [tools] ksck improvements [1/n]: Eliminate KsckMaster class
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck-test.cc@124
PS2, Line 124: using KsckCluster::tables_;
: using KsckCluster::tablet_servers_;
> In this case, I think so, because what we are saying is "hey, this is MockK
Gotcha, thanks for the explanation.
--
To view, visit http://gerrit.cloudera.org:8080/9881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I21f9e244b6ba10e11327c744abc1ac72a9b2ab1c
Gerrit-Change-Number: 9881
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
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: Thu, 05 Apr 2018 17:50:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck improvements [1/n]: Eliminate KsckMaster 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/9881 )
Change subject: [tools] ksck improvements [1/n]: Eliminate KsckMaster class
......................................................................
[tools] ksck improvements [1/n]: Eliminate KsckMaster class
ksck has a KsckCluster and a KsckMaster class. The KsckCluster class's
methods more-or-less map one-to-one with KsckMaster class methods.
The KsckMaster class represents the collective masters of a cluster,
not an individual master.
This patch combines the two classes into a KsckCluster class. This
class is the base class for a MockKsckCluster class and a
RemoteKsckCluster class, for tests and "real life", respectively.
This is just refactoring. There are no functional changes in this patch.
Change-Id: I21f9e244b6ba10e11327c744abc1ac72a9b2ab1c
Reviewed-on: http://gerrit.cloudera.org:8080/9881
Reviewed-by: Attila Bukor <ab...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Will Berkeley <wd...@gmail.com>
---
M src/kudu/client/schema.h
M src/kudu/integration-tests/cluster_verifier.cc
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_tablet.cc
10 files changed, 118 insertions(+), 183 deletions(-)
Approvals:
Attila Bukor: Looks good to me, but someone else must approve
Andrew Wong: Looks good to me, approved
Will Berkeley: Verified
--
To view, visit http://gerrit.cloudera.org:8080/9881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I21f9e244b6ba10e11327c744abc1ac72a9b2ab1c
Gerrit-Change-Number: 9881
Gerrit-PatchSet: 5
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] ksck improvements [1/n]: Eliminate KsckMaster class
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9881 )
Change subject: [tools] ksck improvements [1/n]: Eliminate KsckMaster class
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck-test.cc@124
PS2, Line 124: using KsckCluster::tables_;
: using KsckCluster::tablet_servers_;
> Interesting, I hadn't seen this before. Is this preferred over `friend clas
In this case, I think so, because what we are saying is "hey, this is MockKsckCluster, and any and all tests are free to jiggle its internals to set up their test cases". FRIEND_TEST does that on a test-by-test basis, and to me comes with the connotation that its just a few special tests getting access to a "real" class. Here the entire point is that a MockKsckCluster exists to be arranged suitably to test for the right output from ksck.
--
To view, visit http://gerrit.cloudera.org:8080/9881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I21f9e244b6ba10e11327c744abc1ac72a9b2ab1c
Gerrit-Change-Number: 9881
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
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, 03 Apr 2018 21:07:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck improvements [1/n]: Eliminate KsckMaster class
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/9881 )
Change subject: [tools] ksck improvements [1/n]: Eliminate KsckMaster class
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck-test.cc@124
PS2, Line 124: using KsckCluster::tables_;
: using KsckCluster::tablet_servers_;
> What is this sorcery?!
does it make the parents protected members public? KsckCluster::tables_ seems unused btw.
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck_remote.h
File src/kudu/tools/ksck_remote.h:
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck_remote.h@a102
PS2, Line 102:
don't we need a dtor in the RemoteKsckCluster?
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck_remote.h@104
PS2, Line 104: virtual Status RetrieveTabletServers() OVERRIDE;
now that these are rewritten, I believe these should be changed to C++11 style lowercase overrides.
--
To view, visit http://gerrit.cloudera.org:8080/9881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I21f9e244b6ba10e11327c744abc1ac72a9b2ab1c
Gerrit-Change-Number: 9881
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
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, 03 Apr 2018 12:24:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck improvements [1/n]: Eliminate KsckMaster class
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Attila Bukor, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9881
to look at the new patch set (#3).
Change subject: [tools] ksck improvements [1/n]: Eliminate KsckMaster class
......................................................................
[tools] ksck improvements [1/n]: Eliminate KsckMaster class
ksck has a KsckCluster and a KsckMaster class. The KsckCluster class's
methods more-or-less map one-to-one with KsckMaster class methods.
The KsckMaster class represents the collective masters of a cluster,
not an individual master.
This patch combines the two classes into a KsckCluster class. This
class is the base class for a MockKsckCluster class and a
RemoteKsckCluster class, for tests and "real life", respectively.
This is just refactoring. There are no functional changes in this patch.
Change-Id: I21f9e244b6ba10e11327c744abc1ac72a9b2ab1c
---
M src/kudu/client/schema.h
M src/kudu/integration-tests/cluster_verifier.cc
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_tablet.cc
10 files changed, 118 insertions(+), 182 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/9881/3
--
To view, visit http://gerrit.cloudera.org:8080/9881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I21f9e244b6ba10e11327c744abc1ac72a9b2ab1c
Gerrit-Change-Number: 9881
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
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 improvements [1/n]: Eliminate KsckMaster class
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9881 )
Change subject: [tools] ksck improvements [1/n]: Eliminate KsckMaster class
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck-test.cc@124
PS2, Line 124: using KsckCluster::tables_;
: using KsckCluster::tablet_servers_;
> Yes, it makes the base class's protected tables_ and tablet_servers_ member
Interesting, I hadn't seen this before. Is this preferred over `friend class`es and `FRIEND_TEST`s?
--
To view, visit http://gerrit.cloudera.org:8080/9881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I21f9e244b6ba10e11327c744abc1ac72a9b2ab1c
Gerrit-Change-Number: 9881
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
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, 03 Apr 2018 21:03:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck improvements [1/n]: Eliminate KsckMaster class
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9881 )
Change subject: [tools] ksck improvements [1/n]: Eliminate KsckMaster class
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9881/1/src/kudu/tools/ksck_remote.h
File src/kudu/tools/ksck_remote.h:
http://gerrit.cloudera.org:8080/#/c/9881/1/src/kudu/tools/ksck_remote.h@113
PS1, Line 113: : master_addresses_(std::move(master_addresses)),
> warning: initializer for base class 'kudu::tools::KsckCluster' is redundant
Done
--
To view, visit http://gerrit.cloudera.org:8080/9881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I21f9e244b6ba10e11327c744abc1ac72a9b2ab1c
Gerrit-Change-Number: 9881
Gerrit-PatchSet: 2
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: Sun, 01 Apr 2018 22:34:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck improvements [1/n]: Eliminate KsckMaster class
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Attila Bukor, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9881
to look at the new patch set (#4).
Change subject: [tools] ksck improvements [1/n]: Eliminate KsckMaster class
......................................................................
[tools] ksck improvements [1/n]: Eliminate KsckMaster class
ksck has a KsckCluster and a KsckMaster class. The KsckCluster class's
methods more-or-less map one-to-one with KsckMaster class methods.
The KsckMaster class represents the collective masters of a cluster,
not an individual master.
This patch combines the two classes into a KsckCluster class. This
class is the base class for a MockKsckCluster class and a
RemoteKsckCluster class, for tests and "real life", respectively.
This is just refactoring. There are no functional changes in this patch.
Change-Id: I21f9e244b6ba10e11327c744abc1ac72a9b2ab1c
---
M src/kudu/client/schema.h
M src/kudu/integration-tests/cluster_verifier.cc
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_tablet.cc
10 files changed, 118 insertions(+), 183 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/9881/4
--
To view, visit http://gerrit.cloudera.org:8080/9881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I21f9e244b6ba10e11327c744abc1ac72a9b2ab1c
Gerrit-Change-Number: 9881
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
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 improvements [1/n]: Eliminate KsckMaster class
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/9881 )
Change subject: [tools] ksck improvements [1/n]: Eliminate KsckMaster class
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/9881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I21f9e244b6ba10e11327c744abc1ac72a9b2ab1c
Gerrit-Change-Number: 9881
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
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: Thu, 05 Apr 2018 06:40:44 +0000
Gerrit-HasComments: No
[kudu-CR] [tools] ksck improvements [1/n]: Eliminate KsckMaster 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/9881
to look at the new patch set (#2).
Change subject: [tools] ksck improvements [1/n]: Eliminate KsckMaster class
......................................................................
[tools] ksck improvements [1/n]: Eliminate KsckMaster class
ksck has a KsckCluster and a KsckMaster class. The KsckCluster class's
methods more-or-less map one-to-one with KsckMaster class methods.
The KsckMaster class represents the collective masters of a cluster,
not an individual master.
This patch combines the two classes into a KsckCluster class. This
class is the base class for a MockKsckCluster class and a
RemoteKsckCluster class, for tests and "real life", respectively.
This is just refactoring. There are no functional changes in this patch.
Change-Id: I21f9e244b6ba10e11327c744abc1ac72a9b2ab1c
---
M src/kudu/client/schema.h
M src/kudu/integration-tests/cluster_verifier.cc
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_tablet.cc
10 files changed, 112 insertions(+), 177 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/9881/2
--
To view, visit http://gerrit.cloudera.org:8080/9881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I21f9e244b6ba10e11327c744abc1ac72a9b2ab1c
Gerrit-Change-Number: 9881
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [tools] ksck improvements [1/n]: Eliminate KsckMaster class
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9881 )
Change subject: [tools] ksck improvements [1/n]: Eliminate KsckMaster class
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck-test.cc@124
PS2, Line 124: using KsckCluster::tables_;
: using KsckCluster::tablet_servers_;
What is this sorcery?!
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck_remote.cc@348
PS2, Line 348: tablet_servers_.clear();
: for (const auto* s : servers) {
: shared_ptr<RemoteKsckTabletServer> ts(
: new RemoteKsckTabletServer(s->uuid(),
: HostPort(s->hostname(), s->port()),
: messenger_));
: RETURN_NOT_OK(ts->Init());
: InsertOrDie(&tablet_servers_, ts->uuid(), ts);
: }
This updates tablet_servers_ even if any Init() returns an error. We should probably be creating a local map and swapping at the end.
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck_remote.cc@374
PS2, Line 374: tables_.assign(tables_temp.begin(), tables_temp.end());
We should be able to do `tables_.swap(tables_temp)` here, no?
--
To view, visit http://gerrit.cloudera.org:8080/9881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I21f9e244b6ba10e11327c744abc1ac72a9b2ab1c
Gerrit-Change-Number: 9881
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
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, 03 Apr 2018 03:54:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck improvements [1/n]: Eliminate KsckMaster class
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9881 )
Change subject: [tools] ksck improvements [1/n]: Eliminate KsckMaster class
......................................................................
Patch Set 4: Verified+1
Unrelated flake
--
To view, visit http://gerrit.cloudera.org:8080/9881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I21f9e244b6ba10e11327c744abc1ac72a9b2ab1c
Gerrit-Change-Number: 9881
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 05 Apr 2018 18:20:21 +0000
Gerrit-HasComments: No
[kudu-CR] [tools] ksck improvements [1/n]: Eliminate KsckMaster class
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/9881 )
Change subject: [tools] ksck improvements [1/n]: Eliminate KsckMaster class
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/9881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I21f9e244b6ba10e11327c744abc1ac72a9b2ab1c
Gerrit-Change-Number: 9881
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] ksck improvements [1/n]: Eliminate KsckMaster class
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9881 )
Change subject: [tools] ksck improvements [1/n]: Eliminate KsckMaster class
......................................................................
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck-test.cc@124
PS2, Line 124: using KsckCluster::tables_;
: using KsckCluster::tablet_servers_;
> What is this sorcery?!
It makes KsckCluster::tables_ and KsckCluster::tablet_servers_, which are a protected members of KsckCluster, into public members of MockKsckCluster, so that unit tests can change them directly.
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck-test.cc@124
PS2, Line 124: using KsckCluster::tables_;
: using KsckCluster::tablet_servers_;
> does it make the parents protected members public? KsckCluster::tables_ see
Yes, it makes the base class's protected tables_ and tablet_servers_ members public.
tables_ is used in CreateAndAddTable.
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck_remote.h
File src/kudu/tools/ksck_remote.h:
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck_remote.h@a102
PS2, Line 102:
> don't we need a dtor in the RemoteKsckCluster?
No, I think the implicitly implemented one is fine.
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck_remote.h@104
PS2, Line 104: virtual Status RetrieveTabletServers() override;
> now that these are rewritten, I believe these should be changed to C++11 st
Done
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck_remote.cc@348
PS2, Line 348: TSMap tablet_servers;
: for (const auto* s : servers) {
: shared_ptr<RemoteKsckTabletServer> ts(
: new RemoteKsckTabletServer(s->uuid(),
: HostPort(s->hostname(), s->port()),
: messenger_));
: RETURN_NOT_OK(ts->Init());
: InsertOrDie(&tablet_servers, ts->uuid(), ts);
: }
> This updates tablet_servers_ even if any Init() returns an error. We should
Done
http://gerrit.cloudera.org:8080/#/c/9881/2/src/kudu/tools/ksck_remote.cc@374
PS2, Line 374: }
> We should be able to do `tables_.swap(tables_temp)` here, no?
Done
--
To view, visit http://gerrit.cloudera.org:8080/9881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I21f9e244b6ba10e11327c744abc1ac72a9b2ab1c
Gerrit-Change-Number: 9881
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
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, 03 Apr 2018 20:45:01 +0000
Gerrit-HasComments: Yes