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 2018/10/07 04:31:18 UTC

[kudu-CR] [tests] location assignment for ExternaMiniCluster

Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11606


Change subject: [tests] location assignment for ExternaMiniCluster
......................................................................

[tests] location assignment for ExternaMiniCluster

Introduced location assignment rules for tablet servers running as
part of ExternalMiniCluster.  With this changelist, it's possible
to have tablet servers of ExternalMiniCluster to be assigned among
locations in accordance with simple number-of-tablet-servers-at-location
mappings.

For example, the following is a self-descriptive example of a such
a mapping to assign six tablet servers in a cluster among three
locations:

  L0:1  L1:2  L2:3

Added an integration test to verify the location assignment
rules for ExternalMiniCluster work as intended.

Change-Id: I63309804cf2fdd8a620b50abdd7133af6a033c30
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
A src/kudu/integration-tests/scripts/assign-location.py
A src/kudu/integration-tests/ts_location_assignment-itest.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
8 files changed, 416 insertions(+), 5 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/11606/1
-- 
To view, visit http://gerrit.cloudera.org:8080/11606
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I63309804cf2fdd8a620b50abdd7133af6a033c30
Gerrit-Change-Number: 11606
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>

[kudu-CR] [tests] location assignment for ExternaMiniCluster

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11606 )

Change subject: [tests] location assignment for ExternaMiniCluster
......................................................................


Patch Set 5: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I63309804cf2fdd8a620b50abdd7133af6a033c30
Gerrit-Change-Number: 11606
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 09 Oct 2018 17:37:33 +0000
Gerrit-HasComments: No

[kudu-CR] [tests] location assignment for ExternaMiniCluster

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/11606

to look at the new patch set (#4).

Change subject: [tests] location assignment for ExternaMiniCluster
......................................................................

[tests] location assignment for ExternaMiniCluster

Introduced location assignment rules for tablet servers running as
part of ExternalMiniCluster.  With this changelist, it's possible
to have tablet servers of ExternalMiniCluster to be spread among
locations in accordance with simple number-of-servers-at-location
mapping rules.

For example, the following is a self-descriptive example of such
mapping rules set to spread six tablet servers in a cluster among
three locations:

  L0:1  L1:2  L2:3

Added an integration test to verify the location assignment
rules for ExternalMiniCluster work as intended.

Change-Id: I63309804cf2fdd8a620b50abdd7133af6a033c30
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
A src/kudu/integration-tests/scripts/assign-location.py
A src/kudu/integration-tests/ts_location_assignment-itest.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
8 files changed, 429 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/11606/4
-- 
To view, visit http://gerrit.cloudera.org:8080/11606
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I63309804cf2fdd8a620b50abdd7133af6a033c30
Gerrit-Change-Number: 11606
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] [tests] location assignment for ExternaMiniCluster

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11606 )

Change subject: [tests] location assignment for ExternaMiniCluster
......................................................................


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/11606/2/src/kudu/integration-tests/ts_location_assignment-itest.cc
File src/kudu/integration-tests/ts_location_assignment-itest.cc:

http://gerrit.cloudera.org:8080/#/c/11606/2/src/kudu/integration-tests/ts_location_assignment-itest.cc@39
PS2, Line 39: using kudu::cluster::ExternalTabletServer;
> warning: using decl 'ExternalTabletServer' is unused [misc-unused-using-dec
Done


http://gerrit.cloudera.org:8080/#/c/11606/2/src/kudu/integration-tests/ts_location_assignment-itest.cc@74
PS2, Line 74:         EmplaceOrDie(&info, std::move(location), 1);
> warning: std::move of the const variable 'location' has no effect; remove s
Done


http://gerrit.cloudera.org:8080/#/c/11606/2/src/kudu/integration-tests/ts_location_assignment-itest.cc@81
PS2, Line 81:       EmplaceOrDie(&info, std::move(location), num);
> warning: std::move of the const variable 'location' has no effect; remove s
Done


http://gerrit.cloudera.org:8080/#/c/11606/2/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:

http://gerrit.cloudera.org:8080/#/c/11606/2/src/kudu/master/ts_descriptor.cc@71
PS2, Line 71: using std::make_shared;
> warning: using decl 'make_shared' is unused [misc-unused-using-decls]
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I63309804cf2fdd8a620b50abdd7133af6a033c30
Gerrit-Change-Number: 11606
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 08 Oct 2018 16:55:24 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tests] location assignment for ExternaMiniCluster

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11606 )

Change subject: [tests] location assignment for ExternaMiniCluster
......................................................................

[tests] location assignment for ExternaMiniCluster

Introduced location assignment rules for tablet servers running as
part of ExternalMiniCluster.  With this changelist, it's possible
to have tablet servers of ExternalMiniCluster to be spread among
locations in accordance with simple number-of-servers-at-location
mapping rules.

For example, the following is a self-descriptive example of such
mapping rules set to spread six tablet servers in a cluster among
three locations:

  L0:1  L1:2  L2:3

Added an integration test to verify the location assignment
rules for ExternalMiniCluster work as intended.

Change-Id: I63309804cf2fdd8a620b50abdd7133af6a033c30
Reviewed-on: http://gerrit.cloudera.org:8080/11606
Tested-by: Kudu Jenkins
Reviewed-by: Will Berkeley <wd...@gmail.com>
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
A src/kudu/integration-tests/scripts/assign-location.py
A src/kudu/integration-tests/ts_location_assignment-itest.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
8 files changed, 428 insertions(+), 6 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Will Berkeley: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I63309804cf2fdd8a620b50abdd7133af6a033c30
Gerrit-Change-Number: 11606
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] [tests] location assignment for ExternaMiniCluster

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11606 )

Change subject: [tests] location assignment for ExternaMiniCluster
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11606/1/src/kudu/integration-tests/scripts/assign-location.py
File src/kudu/integration-tests/scripts/assign-location.py:

http://gerrit.cloudera.org:8080/#/c/11606/1/src/kudu/integration-tests/scripts/assign-location.py@135
PS1, Line 135: os.O_EXLOCK
O_EXLOCK is not portable enough.  I'll better use the more-or-less standard scheme with a separate .lock file instead.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I63309804cf2fdd8a620b50abdd7133af6a033c30
Gerrit-Change-Number: 11606
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Sun, 07 Oct 2018 18:58:07 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tests] location assignment for ExternaMiniCluster

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11606 )

Change subject: [tests] location assignment for ExternaMiniCluster
......................................................................


Patch Set 3:

(2 comments)

Looks good! 2 little things.

http://gerrit.cloudera.org:8080/#/c/11606/3/src/kudu/integration-tests/scripts/assign-location.py
File src/kudu/integration-tests/scripts/assign-location.py:

http://gerrit.cloudera.org:8080/#/c/11606/3/src/kudu/integration-tests/scripts/assign-location.py@57
PS3, Line 57: 127.0.0.1
nit: I think you meant 1.2.3.4.


http://gerrit.cloudera.org:8080/#/c/11606/3/src/kudu/integration-tests/ts_location_assignment-itest.cc
File src/kudu/integration-tests/ts_location_assignment-itest.cc:

http://gerrit.cloudera.org:8080/#/c/11606/3/src/kudu/integration-tests/ts_location_assignment-itest.cc@49
PS3, Line 49: // public ExternalMiniClusterITestBase
Why is this commented out?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I63309804cf2fdd8a620b50abdd7133af6a033c30
Gerrit-Change-Number: 11606
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 08 Oct 2018 18:58:14 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tests] location assignment for ExternaMiniCluster

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11606 )

Change subject: [tests] location assignment for ExternaMiniCluster
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11606/3/src/kudu/integration-tests/scripts/assign-location.py
File src/kudu/integration-tests/scripts/assign-location.py:

http://gerrit.cloudera.org:8080/#/c/11606/3/src/kudu/integration-tests/scripts/assign-location.py@57
PS3, Line 57: 127.1.2.3
> nit: I think you meant 1.2.3.4.
Done


http://gerrit.cloudera.org:8080/#/c/11606/3/src/kudu/integration-tests/ts_location_assignment-itest.cc
File src/kudu/integration-tests/ts_location_assignment-itest.cc:

http://gerrit.cloudera.org:8080/#/c/11606/3/src/kudu/integration-tests/ts_location_assignment-itest.cc@49
PS3, Line 49: public KuduTest,
> Why is this commented out?
I realized ExternalMiniClusterITestBase is not that useful here.  I'll remove this commented out line.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I63309804cf2fdd8a620b50abdd7133af6a033c30
Gerrit-Change-Number: 11606
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 08 Oct 2018 20:26:36 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tests] location assignment for ExternaMiniCluster

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11606 )

Change subject: [tests] location assignment for ExternaMiniCluster
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11606/3/src/kudu/mini-cluster/external_mini_cluster.h
File src/kudu/mini-cluster/external_mini_cluster.h:

http://gerrit.cloudera.org:8080/#/c/11606/3/src/kudu/mini-cluster/external_mini_cluster.h@168
PS3, Line 168: form
> Extra "form".
Done


http://gerrit.cloudera.org:8080/#/c/11606/3/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/11606/3/src/kudu/mini-cluster/external_mini_cluster.cc@386
PS3, Line 386: auto location_cmd = Substitute("$0 --state_store=$1",
             :                                    mapping_script_path, state_store_fpath);
> I think this wouldn't work in a multimaster setup, since the masters will s
This works -- the sequencer is designed to allow multiple callers to call the script concurrently.  Please take a closer look at the assign-location.py.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I63309804cf2fdd8a620b50abdd7133af6a033c30
Gerrit-Change-Number: 11606
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 08 Oct 2018 18:06:56 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tests] location assignment for ExternaMiniCluster

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/11606

to look at the new patch set (#5).

Change subject: [tests] location assignment for ExternaMiniCluster
......................................................................

[tests] location assignment for ExternaMiniCluster

Introduced location assignment rules for tablet servers running as
part of ExternalMiniCluster.  With this changelist, it's possible
to have tablet servers of ExternalMiniCluster to be spread among
locations in accordance with simple number-of-servers-at-location
mapping rules.

For example, the following is a self-descriptive example of such
mapping rules set to spread six tablet servers in a cluster among
three locations:

  L0:1  L1:2  L2:3

Added an integration test to verify the location assignment
rules for ExternalMiniCluster work as intended.

Change-Id: I63309804cf2fdd8a620b50abdd7133af6a033c30
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
A src/kudu/integration-tests/scripts/assign-location.py
A src/kudu/integration-tests/ts_location_assignment-itest.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
8 files changed, 428 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/11606/5
-- 
To view, visit http://gerrit.cloudera.org:8080/11606
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I63309804cf2fdd8a620b50abdd7133af6a033c30
Gerrit-Change-Number: 11606
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] [tests] location assignment for ExternaMiniCluster

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/11606

to look at the new patch set (#3).

Change subject: [tests] location assignment for ExternaMiniCluster
......................................................................

[tests] location assignment for ExternaMiniCluster

Introduced location assignment rules for tablet servers running as
part of ExternalMiniCluster.  With this changelist, it's possible
to have tablet servers of ExternalMiniCluster to be spread among
locations in accordance with simple number-of-servers-at-location
mapping rules.

For example, the following is a self-descriptive example of such
mapping rules set to spread six tablet servers in a cluster among
three locations:

  L0:1  L1:2  L2:3

Added an integration test to verify the location assignment
rules for ExternalMiniCluster work as intended.

Change-Id: I63309804cf2fdd8a620b50abdd7133af6a033c30
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
A src/kudu/integration-tests/scripts/assign-location.py
A src/kudu/integration-tests/ts_location_assignment-itest.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
8 files changed, 430 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/11606/3
-- 
To view, visit http://gerrit.cloudera.org:8080/11606
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I63309804cf2fdd8a620b50abdd7133af6a033c30
Gerrit-Change-Number: 11606
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] [tests] location assignment for ExternaMiniCluster

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/11606

to look at the new patch set (#2).

Change subject: [tests] location assignment for ExternaMiniCluster
......................................................................

[tests] location assignment for ExternaMiniCluster

Introduced location assignment rules for tablet servers running as
part of ExternalMiniCluster.  With this changelist, it's possible
to have tablet servers of ExternalMiniCluster to be spread among
locations in accordance with simple number-of-servers-at-location
mapping rules.

For example, the following is a self-descriptive example of such
mapping rules set to spread six tablet servers in a cluster among
three locations:

  L0:1  L1:2  L2:3

Added an integration test to verify the location assignment
rules for ExternalMiniCluster work as intended.

Change-Id: I63309804cf2fdd8a620b50abdd7133af6a033c30
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
A src/kudu/integration-tests/scripts/assign-location.py
A src/kudu/integration-tests/ts_location_assignment-itest.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
8 files changed, 431 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/11606/2
-- 
To view, visit http://gerrit.cloudera.org:8080/11606
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I63309804cf2fdd8a620b50abdd7133af6a033c30
Gerrit-Change-Number: 11606
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] [tests] location assignment for ExternaMiniCluster

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11606 )

Change subject: [tests] location assignment for ExternaMiniCluster
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11606/3/src/kudu/mini-cluster/external_mini_cluster.h
File src/kudu/mini-cluster/external_mini_cluster.h:

http://gerrit.cloudera.org:8080/#/c/11606/3/src/kudu/mini-cluster/external_mini_cluster.h@168
PS3, Line 168: form
Extra "form".


http://gerrit.cloudera.org:8080/#/c/11606/3/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/11606/3/src/kudu/mini-cluster/external_mini_cluster.cc@386
PS3, Line 386: auto location_cmd = Substitute("$0 --state_store=$1",
             :                                    mapping_script_path, state_store_fpath);
I think this wouldn't work in a multimaster setup, since the masters will share a state store and race registering tablet servers. It's a quirk of how we have all TS register with all masters.

Ideally we'd give each master its own state store, but I think it'd suffice to check that the EMC location assignment feature is only used with a single master.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I63309804cf2fdd8a620b50abdd7133af6a033c30
Gerrit-Change-Number: 11606
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 08 Oct 2018 17:59:12 +0000
Gerrit-HasComments: Yes