You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2017/03/18 00:58:00 UTC
[kudu-CR] [kudu-client] extracted startTabletServers() method
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6421
Change subject: [kudu-client] extracted startTabletServers() method
......................................................................
[kudu-client] extracted startTabletServers() method
Minor clean-up on the MiniKuduCluster class:
* renamed startMasters() into startMasterServers()
* extracted the code to start tablet servers into private
startTabletServers() method to pair it with the startMasterServers()
method.
Change-Id: I03e697ea6665e4fd6e981701c6122cccd5784601
---
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
1 file changed, 108 insertions(+), 90 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/6421/1
--
To view, visit http://gerrit.cloudera.org:8080/6421
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I03e697ea6665e4fd6e981701c6122cccd5784601
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [kudu-client] extracted startTabletServers() method
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6421
to look at the new patch set (#2).
Change subject: [kudu-client] extracted startTabletServers() method
......................................................................
[kudu-client] extracted startTabletServers() method
Minor clean-up on the MiniKuduCluster class:
* renamed startMasters() into startMasterServers()
* extracted the code to start tablet servers into private
startTabletServers() method to pair it with the startMasterServers()
method.
Change-Id: I03e697ea6665e4fd6e981701c6122cccd5784601
---
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
1 file changed, 117 insertions(+), 91 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/6421/2
--
To view, visit http://gerrit.cloudera.org:8080/6421
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I03e697ea6665e4fd6e981701c6122cccd5784601
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [kudu-client] extracted startTabletServers() method
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [kudu-client] extracted startTabletServers() method
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/6421/2/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:
PS2, Line 161: master servers
> Likewise: masters
Done
Line 171: private int startMasterServers(int startPort,
> I think the name startMasters() is better; the processes themselves are Kud
Done
PS2, Line 175: if (numServers <= 0) {
: return startPort;
: }
> What's the point of this?
I think method might be called with 0 for numServers, meaning starting no master servers (something like we currently have for tablet servers). Sure, that sort of cluster is not viable, but I think it's worth adding this: i) for symmetry between startMasters() and startTabletServers() ii) in anticipation on a test scenario where minicluster lacks running master server.
PS2, Line 264: if (numServers <= 0) {
: return startPort;
: }
> And this?
There are some test where no tablet servers are started as part of minicluster (that's why some of the test in the first revision failed). So, numServers == 0 is an established use-case for this method.
--
To view, visit http://gerrit.cloudera.org:8080/6421
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I03e697ea6665e4fd6e981701c6122cccd5784601
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [kudu-client] extracted startTabletServers() method
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [kudu-client] extracted startTabletServers() method
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6421
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I03e697ea6665e4fd6e981701c6122cccd5784601
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [kudu-client] extracted startTabletServers() method
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6421
to look at the new patch set (#4).
Change subject: [kudu-client] extracted startTabletServers() method
......................................................................
[kudu-client] extracted startTabletServers() method
Minor clean-up on the MiniKuduCluster class: extracted the code to
start tablet servers into private startTabletServers() method to pair
it with the startMasters() method.
Change-Id: I03e697ea6665e4fd6e981701c6122cccd5784601
---
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
1 file changed, 118 insertions(+), 90 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/6421/4
--
To view, visit http://gerrit.cloudera.org:8080/6421
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I03e697ea6665e4fd6e981701c6122cccd5784601
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [kudu-client] extracted startTabletServers() method
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged.
Change subject: [kudu-client] extracted startTabletServers() method
......................................................................
[kudu-client] extracted startTabletServers() method
Minor clean-up on the MiniKuduCluster class: extracted the code to
start tablet servers into private startTabletServers() method to pair
it with the startMasters() method.
Change-Id: I03e697ea6665e4fd6e981701c6122cccd5784601
Reviewed-on: http://gerrit.cloudera.org:8080/6421
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
1 file changed, 118 insertions(+), 90 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/6421
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I03e697ea6665e4fd6e981701c6122cccd5784601
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [kudu-client] extracted startTabletServers() method
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [kudu-client] extracted startTabletServers() method
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/6421/2/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:
PS2, Line 161: master servers
Likewise: masters
Line 171: private int startMasterServers(int startPort,
I think the name startMasters() is better; the processes themselves are Kudu Masters, not Kudu Master Servers. That's the case everywhere in our code base.
PS2, Line 175: if (numServers <= 0) {
: return startPort;
: }
What's the point of this?
PS2, Line 264: if (numServers <= 0) {
: return startPort;
: }
And this?
--
To view, visit http://gerrit.cloudera.org:8080/6421
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I03e697ea6665e4fd6e981701c6122cccd5784601
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes