[kudu-CR] mini cluster: support multiple data dirs

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: mini_cluster: support multiple data dirs
..


Patch Set 17: Verified+1

Looks like a flaky java test, overriding jenkins

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] mini cluster: support multiple data dirs

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: mini_cluster: support multiple data dirs
..


mini_cluster: support multiple data dirs

This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. A WAL dir will
be generated with an appropriate suffix.

The setup for MiniMaster is also changed to more closely resemble
MiniTabletServer. MasterOptions are generated in the constructor and a
single Start() call will create the Master.

The original directory structure is kept as a default.

E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data-0
/test_dir/test_tserver/data-1
/test_dir/test_tserver/data-2

Original:
/test_dir/test_tserver

Tests are added to the new mini_tablet_server-test and mini_master-test
to exercise this.

Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Reviewed-on: http://gerrit.cloudera.org:8080/7211
Reviewed-by: Mike Percy 
Tested-by: Mike Percy 
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master-test.cc
A src/kudu/master/mini_master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/CMakeLists.txt
A src/kudu/tserver/mini_tablet_server-test.cc
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
16 files changed, 233 insertions(+), 122 deletions(-)

Approvals:
  Mike Percy: Looks good to me, approved; Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] mini cluster: support multiple data dirs

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: mini_cluster: support multiple data dirs
..


Patch Set 17: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] mini cluster: support multiple data dirs

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: mini_cluster: support multiple data dirs
..


Patch Set 17:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7211/16/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:

Line 46: MiniMaster::MiniMaster(string fs_root, HostPort rpc_bind_addr, int 
num_data_dirs)
> warning: pass by value and use std::move [modernize-pass-by-value]
Done


http://gerrit.cloudera.org:8080/#/c/7211/16/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:

Line 50: MiniTabletServer::MiniTabletServer(string fs_root,
> warning: pass by value and use std::move [modernize-pass-by-value]
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] mini cluster: support multiple data dirs

[Andrew Wong (Code Review)]

Hello Mike Percy, Kudu Jenkins,

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

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

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

Change subject: mini_cluster: support multiple data dirs
..

mini_cluster: support multiple data dirs

This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. A WAL dir will
be generated with an appropriate suffix.

The setup for MiniMaster is also changed to more closely resemble
MiniTabletServer. MasterOptions are generated in the constructor and a
single Start() call will create the Master.

The original directory structure is kept as a default.

E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data-0
/test_dir/test_tserver/data-1
/test_dir/test_tserver/data-2

Original:
/test_dir/test_tserver

Tests are added to the new mini_tablet_server-test and mini_master-test
to exercise this.

Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master-test.cc
A src/kudu/master/mini_master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/CMakeLists.txt
A src/kudu/tserver/mini_tablet_server-test.cc
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
16 files changed, 233 insertions(+), 122 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7211/17
-- 
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] mini cluster: support multiple data dirs

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: mini_cluster: support multiple data dirs
..


Patch Set 16:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7211/15/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:

Line 84:   RETURN_NOT_OK(env_util::CreateDirIfMissing(Env::Default(), 
fs_root_));
> It seems counterintuitive that we implicitly treat opts_.fs_opts.wal_path a
Hrm, for multiple wal dirs, I'd think that they'd still fall within a single 
minimaster root, but I agree it feels weird getting the root this way. Done


PS15, Line 85: );
> nit: indentation and also you can leave this bool out if you don't use it (
Done


http://gerrit.cloudera.org:8080/#/c/7211/15/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:

Line 82:   RETURN_NOT_OK(env_util::CreateDirIfMissing(Env::Default(), 
fs_root_));
> Seems better to have an fs_root_ member here than piggyback on wal_path as 
Done


Line 83:   unique_ptr server(new TabletServer(opts_));
> nit: bool is optional
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] mini cluster: support multiple data dirs

[Andrew Wong (Code Review)]

Hello Mike Percy, Kudu Jenkins,

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

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

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

Change subject: mini_cluster: support multiple data dirs
..

mini_cluster: support multiple data dirs

This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. A WAL dir will
be generated with an appropriate suffix.

The setup for MiniMaster is also changed to more closely resemble
MiniTabletServer. MasterOptions are generated in the constructor and a
single Start() call will create the Master.

The original directory structure is kept as a default.

E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data-0
/test_dir/test_tserver/data-1
/test_dir/test_tserver/data-2

Original:
/test_dir/test_tserver

Tests are added to the new mini_tablet_server-test and mini_master-test
to exercise this.

Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master-test.cc
A src/kudu/master/mini_master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/CMakeLists.txt
A src/kudu/tserver/mini_tablet_server-test.cc
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
16 files changed, 232 insertions(+), 121 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7211/16
-- 
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] mini cluster: support multiple data dirs

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: mini_cluster: support multiple data dirs
..


Patch Set 15: Code-Review+1

(4 comments)

Looks good, just a couple nits

http://gerrit.cloudera.org:8080/#/c/7211/15/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:

Line 84:   RETURN_NOT_OK(env_util::CreateDirIfMissing(Env::Default(), 
DirName(opts_.fs_opts.wal_path),
It seems counterintuitive that we implicitly treat opts_.fs_opts.wal_path as 
the fs_root dir. I think we should keep fs_root_ as a member.

This will also matter once we add support for multiple wal dirs so better to 
just leave it as it was now.


PS15, Line 85:  
nit: indentation and also you can leave this bool out if you don't use it (it 
defaults to nullptr)


http://gerrit.cloudera.org:8080/#/c/7211/15/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:

Line 82:   RETURN_NOT_OK(env_util::CreateDirIfMissing(Env::Default(), 
DirName(opts_.fs_opts.wal_path),
Seems better to have an fs_root_ member here than piggyback on wal_path as 
noted in the other file


Line 83:   ));
nit: bool is optional


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] mini cluster: support multiple data dirs

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: mini_cluster: support multiple data dirs
..


Patch Set 15:

The failure seems unrelated

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] mini cluster: support multiple data dirs

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: mini_cluster: support multiple data dirs
..

mini_cluster: support multiple data dirs

This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. A WAL dir will
be generated with an appropriate suffix.

The setup for MiniMaster is also changed to more closely resemble
MiniTabletServer. MasterOptions are generated in the constructor and a
single Start() call will create the Master.

The original directory structure is kept as a default.

E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data-0
/test_dir/test_tserver/data-1
/test_dir/test_tserver/data-2

Original:
/test_dir/test_tserver

Tests are added to the new mini_tablet_server-test and mini_master-test
to exercise this.

Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master-test.cc
A src/kudu/master/mini_master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/CMakeLists.txt
A src/kudu/tserver/mini_tablet_server-test.cc
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
16 files changed, 232 insertions(+), 120 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7211/15
-- 
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] mini cluster: support multiple data dirs

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: mini_cluster: support multiple data dirs
..


Patch Set 14:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/integration-tests/internal_mini_cluster.cc
File src/kudu/integration-tests/internal_mini_cluster.cc:

PS11, Line 60: opts_(st
> Done, but it came at the cost of making opts_ non-const.
great - seems alright to me


http://gerrit.cloudera.org:8080/#/c/7211/14/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:

Line 60: CHECK_OK(env_util::CreateDirIfMissing(Env::Default(), fs_root_, 
));
Are we sure this step is required now but not previously?

If it is required now, perhaps this should be RETURN_NOT_OK() in Start() 
instead of a CHECK() in the constructor since it's not there to enforce a 
particular API, ensure correctness, or run on a non-main thread.


Line 102:   Shutdown();
Shutdown() still here in Restart()


http://gerrit.cloudera.org:8080/#/c/7211/14/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:

Line 65: CHECK_OK(env_util::CreateDirIfMissing(Env::Default(), fs_root, 
));
See comment in mini_master.cc


http://gerrit.cloudera.org:8080/#/c/7211/14/src/kudu/tserver/tablet_server-test-base.h
File src/kudu/tserver/tablet_server-test-base.h:

Line 103: tablet_replica_.reset();
Why is this needed now but not before?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] mini cluster: support multiple data dirs

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: mini_cluster: support multiple data dirs
..

mini_cluster: support multiple data dirs

This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. A WAL dir will
be generated with an appropriate suffix.

The setup for MiniMaster is also changed to more closely resemble
MiniTabletServer. MasterOptions are generated in the constructor and a
single Start() call will create the Master.

The original directory structure is kept as a default.

E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data-0
/test_dir/test_tserver/data-1
/test_dir/test_tserver/data-2

Original:
/test_dir/test_tserver

Tests are added to the new mini_tablet_server-test and mini_master-test
to exercise this.

Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master-test.cc
A src/kudu/master/mini_master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/CMakeLists.txt
A src/kudu/tserver/mini_tablet_server-test.cc
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
16 files changed, 226 insertions(+), 114 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7211/14
-- 
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] mini cluster: support multiple data dirs

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: mini_cluster: support multiple data dirs
..


Patch Set 11:

(14 comments)

http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/integration-tests/internal_mini_cluster.cc
File src/kudu/integration-tests/internal_mini_cluster.cc:

PS11, Line 60: fs_root_
> Can we get rid of this member too? We could put this in the body of the con
Done, but it came at the cost of making opts_ non-const.


http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/master/master-test.cc
File src/kudu/master/master-test.cc:

Line 1349: TEST_F(MasterTest, TestMultiDirMaster) {
> If this is just unit test coverage for the MiniMaster I think it should be 
Done


http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:

PS11, Line 59: data$0
> I think it would be more readable as "data-$0"
Done


Line 70: void MiniMaster::SetMasterAddresses(const vector& 
master_addrs) {
> nit: add CHECK(!master_) so this is only called when it's not running
Done


Line 71:   opts_.master_addresses = master_addrs;
> nit: pass by value and use std::move() here
Done


PS11, Line 85:   if (opts_.master_addresses.empty()) {
 : return master_->WaitForCatalogManagerInit();
 :   }
> Add comment: // Wait for catalog manager to be ready if we only have a sing
Done


Line 92:   opts_.rpc_opts.rpc_bind_addresses = bound_rpc_.ToString();
> add: CHECK(!master_) so this is only called after Shutdown()
Done


PS11, Line 93: HostPort(bound_http_).host()
> no need to construct a HostPort here, just call bound_http_.host() and get 
Done


PS11, Line 94: HostPort(bound_http_).port()
> bound_http_.port()
Done


Line 95:   Shutdown();
> Restart() has unusual semantics in the minicluster* classes. They require S
Ah I see, makes sense. I'll save that for another patch then; I don't have 
anything against those semantics.

To just keep the original behavior (with no Shutdown()), I'll just take out 
Shutdown() from Restart(). In all the tests, if Shutdown() is always called 
beforehand, the Shutdown() here would just be a no-op anyway.


http://gerrit.cloudera.org:8080/#/c/7211/12/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:

Line 100:   if (master_) {
> warning: an integer is interpreted as a character code when assigning it to
Done


http://gerrit.cloudera.org:8080/#/c/7211/12/src/kudu/tserver/mini_tablet_server-test.cc
File src/kudu/tserver/mini_tablet_server-test.cc:

Line 21
> warning: #includes are not sorted properly [llvm-include-order]
Done


http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:

PS11, Line 65: "data$0
> I know this is subjective but I think this would be a little more readable 
Done


http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:

Line 2486: TEST_F(TabletServerTest, TestMultiDirServer) {
> This should probably be in its own file if this is just a unit test for the
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] mini cluster: support multiple data dirs

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: mini_cluster: support multiple data dirs
..

mini_cluster: support multiple data dirs

This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. A WAL dir will
be generated with an appropriate suffix.

The setup for MiniMaster is also changed to more closely resemble
MiniTabletServer. MasterOptions are generated in the constructor and a
single Start() call will create the Master.

The original directory structure is kept as a default.

E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data0
/test_dir/test_tserver/data1
/test_dir/test_tserver/data2

Original:
/test_dir/test_tserver

Tests are added to the new mini_tablet_server-test and mini_master-test
to exercise this.

Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master-test.cc
A src/kudu/master/mini_master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/CMakeLists.txt
A src/kudu/tserver/mini_tablet_server-test.cc
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
16 files changed, 226 insertions(+), 114 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7211/13
-- 
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] mini cluster: support multiple data dirs

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: mini_cluster: support multiple data dirs
..

mini_cluster: support multiple data dirs

This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. A WAL dir will
be generated with an appropriate suffix.

The setup for MiniMaster is also changed to more closely resemble
MiniTabletServer. MasterOptions are generated in the constructor and a
single Start() call will create the Master.

The original directory structure is kept as a default.

E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data0
/test_dir/test_tserver/data1
/test_dir/test_tserver/data2

Original:
/test_dir/test_tserver

Tests are added to the new mini_tablet_server-test and mini_master-test
to exercise this.

Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master-test.cc
A src/kudu/master/mini_master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/CMakeLists.txt
A src/kudu/tserver/mini_tablet_server-test.cc
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
16 files changed, 223 insertions(+), 109 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7211/12
-- 
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] mini cluster: support multiple data dirs

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: mini_cluster: support multiple data dirs
..


Patch Set 11:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:

PS11, Line 59: data$0
I think it would be more readable as "data-$0"


http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:

PS11, Line 65: "data$0
I know this is subjective but I think this would be a little more readable when 
doing an "ls" as "data-$0"


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] mini cluster: support multiple data dirs

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: mini_cluster: support multiple data dirs
..


Patch Set 11:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/integration-tests/internal_mini_cluster.cc
File src/kudu/integration-tests/internal_mini_cluster.cc:

PS11, Line 60: fs_root_
Can we get rid of this member too? We could put this in the body of the 
constructor and finally get rid of this last non-opts member:

  if (opts_.data_root.empty()) {
opts_.data_root = JoinPathSegments(GetTestDataDirectory(), 
"minicluster-data");
  }


http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/master/master-test.cc
File src/kudu/master/master-test.cc:

Line 1349: TEST_F(MasterTest, TestMultiDirMaster) {
If this is just unit test coverage for the MiniMaster I think it should be 
given its own test file, i.e. mini_master-test.cc


http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:

Line 70: void MiniMaster::SetMasterAddresses(const vector& 
master_addrs) {
nit: add CHECK(!master_) so this is only called when it's not running


Line 71:   opts_.master_addresses = master_addrs;
nit: pass by value and use std::move() here


PS11, Line 85:   if (opts_.master_addresses.empty()) {
 : return master_->WaitForCatalogManagerInit();
 :   }
Add comment: // Wait for catalog manager to be ready if we only have a single 
master.

Actually, we should probably make this if (opts_.master_addresses.size() <= 1)


Line 92:   opts_.rpc_opts.rpc_bind_addresses = bound_rpc_.ToString();
add: CHECK(!master_) so this is only called after Shutdown()

See below comment for explanation of why Shutdown() has to be called before 
Restart()


PS11, Line 93: HostPort(bound_http_).host()
no need to construct a HostPort here, just call bound_http_.host() and get the 
host from the Sockaddr directly.


PS11, Line 94: HostPort(bound_http_).port()
bound_http_.port()


Line 95:   Shutdown();
Restart() has unusual semantics in the minicluster* classes. They require 
Shutdown() to be called, then Restart() to be called. Keep this consistent for 
now; if you want to change those semantics later we can consider it in a 
separate patch across Internal and External Minicluster classes.

By the way, Shutdown() is what sets bound_rpc_ and bound_http_ so AFAICT this 
would not really work as written. The fact that it doesn't fail any tests 
probably just indicates a lack of test coverage on MiniMaster.


http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:

Line 2486: TEST_F(TabletServerTest, TestMultiDirServer) {
This should probably be in its own file if this is just a unit test for the 
MiniTabletServer, i.e. mini_tablet_server-test.cc


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] mini cluster: support multiple data dirs

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: mini_cluster: support multiple data dirs
..


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7211/9/src/kudu/integration-tests/internal_mini_cluster.cc
File src/kudu/integration-tests/internal_mini_cluster.cc:

Line 65: master_rpc_ports_(options.master_rpc_ports),
> This looks like a real bug (and good fix) from commit e675873. Can you make
Will do


http://gerrit.cloudera.org:8080/#/c/7211/7/src/kudu/integration-tests/internal_mini_cluster.h
File src/kudu/integration-tests/internal_mini_cluster.h:

Line 201:   const int num_data_dirs_;
> Why? Wouldn't it be simpler to copy opts_ and drop these individual members
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] mini cluster: support multiple data dirs

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: mini_cluster: support multiple data dirs
..

mini_cluster: support multiple data dirs

This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. A WAL dir will
be generated with an appropriate suffix.

The setup for MiniMaster is also changed to more closely resemble
MiniTabletServer. MasterOptions are generated in the constructor and a
single Start() call will create the Master.

The original directory structure is kept as a default.

E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data0
/test_dir/test_tserver/data1
/test_dir/test_tserver/data2

Original:
/test_dir/test_tserver

Tests are added to tablet_server-test and master-test to exercise this.

Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
12 files changed, 141 insertions(+), 106 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] mini cluster: support multiple data dirs

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: mini_cluster: support multiple data dirs
..


Patch Set 9:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7211/9/src/kudu/integration-tests/internal_mini_cluster.cc
File src/kudu/integration-tests/internal_mini_cluster.cc:

Line 65: bind_mode_(options.bind_mode),
This looks like a real bug (and good fix) from commit e675873. Can you make 
sure Mike knows about it?


http://gerrit.cloudera.org:8080/#/c/7211/7/src/kudu/integration-tests/internal_mini_cluster.h
File src/kudu/integration-tests/internal_mini_cluster.h:

Line 201:   const int num_data_dirs_;
> Interestingly we weren't copying opts_. I've removed it in favor of keeping
Why? Wouldn't it be simpler to copy opts_ and drop these individual members?

And with your current approach what's the point of keeping opts_ around if it's 
not used?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] mini cluster: support multiple data dirs

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: mini_cluster: support multiple data dirs
..

mini_cluster: support multiple data dirs

This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. A WAL dir will
be generated with an appropriate suffix.

The setup for MiniMaster is also changed to more closely resemble
MiniTabletServer. MasterOptions are generated in the constructor and a
single Start() call will create the Master.

The original directory structure is kept as a default.

E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data0
/test_dir/test_tserver/data1
/test_dir/test_tserver/data2

Original:
/test_dir/test_tserver

Tests are added to tablet_server-test and master-test to exercise this.

Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
12 files changed, 126 insertions(+), 80 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7211/9
-- 
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] mini cluster: support multiple data dirs

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: mini_cluster: support multiple data dirs
..


Patch Set 7:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/7211/6//COMMIT_MSG
Commit Message:

Line 7: mini_cluster: support multiple data dirs
> Agreed
Done


Line 7: mini_cluster: support multiple data dirs
> I think this would be better defined in MiniClusterOptions and then plumbed
Done


PS6, Line 18: be generated for tservers (a similar one is created for masters):
: /test_dir/test_tserver/wal
: /test_dir/test_tserver/data0
: /test_dir/test_tserver/data1
> Hmm, what do you think about:
Done


PS6, Line 18: be generated for tservers (a similar one is created for masters):
: /test_dir/test_tserver/wal
: /test_dir/test_tserver/data0
: /test_dir/test_tserver/data1
> +1 on this
Done


http://gerrit.cloudera.org:8080/#/c/7211/7/src/kudu/integration-tests/internal_mini_cluster.h
File src/kudu/integration-tests/internal_mini_cluster.h:

Line 201:   const int num_data_dirs_;
> Hmm, doesn't seem like we need this (or fs_root_) since we have a copy of o
Interestingly we weren't copying opts_. I've removed it in favor of keeping 
these private members.


http://gerrit.cloudera.org:8080/#/c/7211/7/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:

Line 110: 
> Not your fault, but could you make 'opts' a class member and set it up in t
Done


http://gerrit.cloudera.org:8080/#/c/7211/6/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:

Line 59:   opts_.webserver_opts.port = 0;
> Don't need std:: prefix.
Done


PS6, Line 60: _data_di
> int here
Done


http://gerrit.cloudera.org:8080/#/c/7211/7/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:

Line 21: #include 
> warning: #includes are not sorted properly [llvm-include-order]
Done


Line 32: #include "kudu/util/path_util.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done


http://gerrit.cloudera.org:8080/#/c/7211/6/src/kudu/tserver/mini_tablet_server.h
File src/kudu/tserver/mini_tablet_server.h:

PS6, Line 43: 
> An int would be fine here too.
Done


http://gerrit.cloudera.org:8080/#/c/7211/6/src/kudu/tserver/tablet_server-test-base.h
File src/kudu/tserver/tablet_server-test-base.h:

PS6, Line 100: 
> Don't use default arguments with virtual functions. They don't really work.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] mini cluster: support multiple data dirs

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: mini_cluster: support multiple data dirs
..

mini_cluster: support multiple data dirs

This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. A WAL dir will
be generated with an appropriate suffix.

The setup for MiniMaster is also changed to more closely resemble
MiniTabletServer. MasterOptions are generated in the constructor and a
single Start() call will create the Master.

The original directory structure is kept as a default.

E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data0
/test_dir/test_tserver/data1
/test_dir/test_tserver/data2

Original:
/test_dir/test_tserver

Tests are added to tablet_server-test and master-test to exercise this.

Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
12 files changed, 127 insertions(+), 84 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7211/8
-- 
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] mini cluster: support multiple data dirs

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: mini_cluster: support multiple data dirs
..


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7211/7/src/kudu/integration-tests/internal_mini_cluster.h
File src/kudu/integration-tests/internal_mini_cluster.h:

Line 201:   const int num_data_dirs_;
Hmm, doesn't seem like we need this (or fs_root_) since we have a copy of opts 
in opts_.


http://gerrit.cloudera.org:8080/#/c/7211/7/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:

Line 110: 
Not your fault, but could you make 'opts' a class member and set it up in the 
constructor, so it's more like MiniTabletServer?

Doing so would open the door to a common MiniServer implementation whose 
constructor can initialize ServerBaseOptions for both MiniMaster and 
MiniTabletServer.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] mini cluster: support multiple data dirs

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: mini_cluster: support multiple data dirs
..

mini_cluster: support multiple data dirs

This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. Additionally,
a WAL dir will be generated with an appropriate suffix. Additionally,
these can both be specified by MiniClusterOptions.

The original directory structure is kept as a default.

E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data0
/test_dir/test_tserver/data1
/test_dir/test_tserver/data2

Original:
/test_dir/test_tserver

Tests are added to tablet_server-test and master-test to exercise this.

Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/master/master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
11 files changed, 84 insertions(+), 17 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7211/7
-- 
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot