You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2017/05/22 22:01:45 UTC
[kudu-CR] mini cluster: Test infrastructure improvements
Hello David Ribeiro Alves, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/6959
to review the following change.
Change subject: mini cluster: Test infrastructure improvements
......................................................................
mini cluster: Test infrastructure improvements
This patch adds a couple small improvements to the MiniCluster infra.
* Add base class helpers to MiniClusterITestBase to avoid code
duplication, including StopCluster() and ts_map_ for convenience and
consistency with ExternalMiniClusterITestBase.
* Add ListTablets() helper function to MiniTabletServer.
Change-Id: I6bc6a04b113e59243fb788fec15b9935c3dcf069
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/delete_tablet-itest.cc
A src/kudu/integration-tests/mini_cluster-itest-base.cc
M src/kudu/integration-tests/mini_cluster-itest-base.h
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
6 files changed, 85 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/6959/1
--
To view, visit http://gerrit.cloudera.org:8080/6959
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6bc6a04b113e59243fb788fec15b9935c3dcf069
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] mini cluster: Test infrastructure improvements
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: mini cluster: Test infrastructure improvements
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/6959/3/src/kudu/integration-tests/mini_cluster-itest-base.cc
File src/kudu/integration-tests/mini_cluster-itest-base.cc:
PS3, Line 20: #include <string>
: #include <vector>
nit: are these necessary?
http://gerrit.cloudera.org:8080/#/c/6959/3/src/kudu/integration-tests/mini_cluster-itest-base.h
File src/kudu/integration-tests/mini_cluster-itest-base.h:
PS3, Line 38: virtual
nit: it's better to drop this since there is 'override'
http://gerrit.cloudera.org:8080/#/c/6959/3/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:
Line 20: #include <utility>
nit: please also add
#include <string>
#include <vector>
using std::string;
using std::vector;
http://gerrit.cloudera.org:8080/#/c/6959/3/src/kudu/tserver/mini_tablet_server.h
File src/kudu/tserver/mini_tablet_server.h:
PS3, Line 26: #include <string>
: #include <vector>
nit: could you move these up to precede the list of kudu-specific headers?
--
To view, visit http://gerrit.cloudera.org:8080/6959
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6bc6a04b113e59243fb788fec15b9935c3dcf069
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] mini cluster: Test infrastructure improvements
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged.
Change subject: mini cluster: Test infrastructure improvements
......................................................................
mini cluster: Test infrastructure improvements
This patch adds a couple small improvements to the MiniCluster infra.
* Add base class helpers to MiniClusterITestBase to avoid code
duplication, including StopCluster() and ts_map_ for convenience and
consistency with ExternalMiniClusterITestBase.
* Add ListTablets() helper function to MiniTabletServer.
Change-Id: I6bc6a04b113e59243fb788fec15b9935c3dcf069
Reviewed-on: http://gerrit.cloudera.org:8080/6959
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/delete_tablet-itest.cc
A src/kudu/integration-tests/internal_mini_cluster-itest-base.cc
M src/kudu/integration-tests/internal_mini_cluster-itest-base.h
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
6 files changed, 81 insertions(+), 24 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/6959
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I6bc6a04b113e59243fb788fec15b9935c3dcf069
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] mini cluster: Test infrastructure improvements
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: mini cluster: Test infrastructure improvements
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6959
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6bc6a04b113e59243fb788fec15b9935c3dcf069
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] mini cluster: Test infrastructure improvements
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: mini cluster: Test infrastructure improvements
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6959/4/src/kudu/integration-tests/delete_tablet-itest.cc
File src/kudu/integration-tests/delete_tablet-itest.cc:
PS4, Line 49: ts_map_[mts->uuid()]
A small nit which I didn't notice on the first pass. Feel free to ignore:
Would it make sense make ts_map_ private and to add a method to get a pointer to the tablet server given its UUID (something like we have in x-mini-clusters?)
Or for some reason we want ts_map_ to be exposed for access and modification by tests scenarios?
--
To view, visit http://gerrit.cloudera.org:8080/6959
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6bc6a04b113e59243fb788fec15b9935c3dcf069
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] mini cluster: Test infrastructure improvements
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6959
to look at the new patch set (#6).
Change subject: mini cluster: Test infrastructure improvements
......................................................................
mini cluster: Test infrastructure improvements
This patch adds a couple small improvements to the MiniCluster infra.
* Add base class helpers to MiniClusterITestBase to avoid code
duplication, including StopCluster() and ts_map_ for convenience and
consistency with ExternalMiniClusterITestBase.
* Add ListTablets() helper function to MiniTabletServer.
Change-Id: I6bc6a04b113e59243fb788fec15b9935c3dcf069
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/delete_tablet-itest.cc
A src/kudu/integration-tests/internal_mini_cluster-itest-base.cc
M src/kudu/integration-tests/internal_mini_cluster-itest-base.h
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
6 files changed, 81 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/6959/6
--
To view, visit http://gerrit.cloudera.org:8080/6959
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6bc6a04b113e59243fb788fec15b9935c3dcf069
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] mini cluster: Test infrastructure improvements
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: mini cluster: Test infrastructure improvements
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/6959/3/src/kudu/integration-tests/mini_cluster-itest-base.cc
File src/kudu/integration-tests/mini_cluster-itest-base.cc:
PS3, Line 20: #include <string>
: #include <vector>
> nit: are these necessary?
Ah, nope. Removed.
http://gerrit.cloudera.org:8080/#/c/6959/3/src/kudu/integration-tests/mini_cluster-itest-base.h
File src/kudu/integration-tests/mini_cluster-itest-base.h:
PS3, Line 38: virtual
> nit: it's better to drop this since there is 'override'
Done
http://gerrit.cloudera.org:8080/#/c/6959/3/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:
Line 20: #include <utility>
> nit: please also add
it doesn't need the includes because they are pulled in by the corresponding header file. But I will add the using expressions
http://gerrit.cloudera.org:8080/#/c/6959/3/src/kudu/tserver/mini_tablet_server.h
File src/kudu/tserver/mini_tablet_server.h:
PS3, Line 26: #include <string>
: #include <vector>
> nit: could you move these up to precede the list of kudu-specific headers?
Done
--
To view, visit http://gerrit.cloudera.org:8080/6959
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6bc6a04b113e59243fb788fec15b9935c3dcf069
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] mini cluster: Test infrastructure improvements
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6959
to look at the new patch set (#4).
Change subject: mini cluster: Test infrastructure improvements
......................................................................
mini cluster: Test infrastructure improvements
This patch adds a couple small improvements to the MiniCluster infra.
* Add base class helpers to MiniClusterITestBase to avoid code
duplication, including StopCluster() and ts_map_ for convenience and
consistency with ExternalMiniClusterITestBase.
* Add ListTablets() helper function to MiniTabletServer.
Change-Id: I6bc6a04b113e59243fb788fec15b9935c3dcf069
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/delete_tablet-itest.cc
A src/kudu/integration-tests/internal_mini_cluster-itest-base.cc
M src/kudu/integration-tests/internal_mini_cluster-itest-base.h
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
6 files changed, 82 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/6959/4
--
To view, visit http://gerrit.cloudera.org:8080/6959
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6bc6a04b113e59243fb788fec15b9935c3dcf069
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] mini cluster: Test infrastructure improvements
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6959
to look at the new patch set (#5).
Change subject: mini cluster: Test infrastructure improvements
......................................................................
mini cluster: Test infrastructure improvements
This patch adds a couple small improvements to the MiniCluster infra.
* Add base class helpers to MiniClusterITestBase to avoid code
duplication, including StopCluster() and ts_map_ for convenience and
consistency with ExternalMiniClusterITestBase.
* Add ListTablets() helper function to MiniTabletServer.
Change-Id: I6bc6a04b113e59243fb788fec15b9935c3dcf069
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/delete_tablet-itest.cc
A src/kudu/integration-tests/internal_mini_cluster-itest-base.cc
M src/kudu/integration-tests/internal_mini_cluster-itest-base.h
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
6 files changed, 82 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/6959/5
--
To view, visit http://gerrit.cloudera.org:8080/6959
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6bc6a04b113e59243fb788fec15b9935c3dcf069
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] mini cluster: Test infrastructure improvements
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: mini cluster: Test infrastructure improvements
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6959/4/src/kudu/integration-tests/delete_tablet-itest.cc
File src/kudu/integration-tests/delete_tablet-itest.cc:
PS4, Line 49: ts_map_[mts->uuid()]
> A small nit which I didn't notice on the first pass. Feel free to ignore:
I'm open to fixing this and eventually I want to get rid of TServerDetails and replace it with a base class MiniTabletServer that works for both internal and external mini clusters. But I don't want to do all of that stuff as part of this patch, if that makes sense.
--
To view, visit http://gerrit.cloudera.org:8080/6959
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6bc6a04b113e59243fb788fec15b9935c3dcf069
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] mini cluster: Test infrastructure improvements
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6959
to look at the new patch set (#3).
Change subject: mini cluster: Test infrastructure improvements
......................................................................
mini cluster: Test infrastructure improvements
This patch adds a couple small improvements to the MiniCluster infra.
* Add base class helpers to MiniClusterITestBase to avoid code
duplication, including StopCluster() and ts_map_ for convenience and
consistency with ExternalMiniClusterITestBase.
* Add ListTablets() helper function to MiniTabletServer.
Change-Id: I6bc6a04b113e59243fb788fec15b9935c3dcf069
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/delete_tablet-itest.cc
A src/kudu/integration-tests/mini_cluster-itest-base.cc
M src/kudu/integration-tests/mini_cluster-itest-base.h
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
6 files changed, 85 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/6959/3
--
To view, visit http://gerrit.cloudera.org:8080/6959
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6bc6a04b113e59243fb788fec15b9935c3dcf069
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>