[kudu-CR] external minicluster: expand EMC dir usage

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: external minicluster: expand EMC dir usage
..


external minicluster: expand EMC dir usage

In order to test different disk configurations, it is becoming
increasingly important to have end-to-end testing with nodes backed
by multiple directories.

External miniclusters by default use a single directory for each
daemon's data (i.e. wals and data dirs fall under a single
/cluster/daemon/data directory). This patch adds multi-directory support
via a new 'num_data_dirs' parameter to ExternalMiniClusterOptions.
Additionally, a 'wal_dir' parameter is added to ExternalDaemonOptions to
separate the wal location from the data directories.

If 'num_data_dirs' is greater than 1, each daemon will generate multiple
paths, appending each with a numeric suffix, up to the number specified.

E.g. EMCs that would have used the path /cluster/data, if specifying
'num_data_dirs' as 3, will now generate multiple data directories
/cluster/data-0, /cluster/data-1, /cluster/data-2. The wal will be added
to /cluster/wal.

The new test multidir-cluster-itest demonstrates this.

This test has been run via dist-test 2000 with no flakes. Results here:
http://dist-test.cloudera.org/job?job_id=awong.1496701042.24862

Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Reviewed-on: http://gerrit.cloudera.org:8080/6845
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
Reviewed-by: Mike Percy 
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/disk_reservation-itest.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.h
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_migration-itest.cc
A src/kudu/integration-tests/multidir_cluster-itest.cc
M src/kudu/integration-tests/open-readonly-fs-itest.cc
M src/kudu/integration-tests/ts_itest-base.h
M src/kudu/integration-tests/ts_recovery-itest.cc
15 files changed, 231 insertions(+), 61 deletions(-)

Approvals:
  Mike Percy: Looks good to me, approved
  Adar Dembo: Looks good to me, but someone else must approve
  Todd Lipcon: Looks good to me, but someone else must approve
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 25
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] external minicluster: expand EMC dir usage

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: external minicluster: expand EMC dir usage
..


Patch Set 24: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 24
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] external minicluster: expand EMC dir usage

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: external minicluster: expand EMC dir usage
..


Patch Set 24: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 24
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] external minicluster: expand EMC dir usage

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: external minicluster: expand EMC dir usage
..


Patch Set 24: Code-Review+1

Deferring to Mike.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 24
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] external minicluster: expand EMC dir usage

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: external minicluster: expand EMC dir usage
..


Patch Set 24:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6845/21/src/kudu/integration-tests/multidir_cluster-itest.cc
File src/kudu/integration-tests/multidir_cluster-itest.cc:

Line 89:   work.StopAndJoin();
> Sorry, I meant to put this into the above ASSERT_EVENTUALLY block. Let's ei
Ah my mistake, I misread your comment. Done, moved it to the above 
ASSERT_EVENTUALLY. Also ran it a couple thousand times and put the results in 
the commit message.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 24
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] external minicluster: expand EMC dir usage

[Andrew Wong (Code Review)]

Hello Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: external minicluster: expand EMC dir usage
..

external minicluster: expand EMC dir usage

In order to test different disk configurations, it is becoming
increasingly important to have end-to-end testing with nodes backed
by multiple directories.

External miniclusters by default use a single directory for each
daemon's data (i.e. wals and data dirs fall under a single
/cluster/daemon/data directory). This patch adds multi-directory support
via a new 'num_data_dirs' parameter to ExternalMiniClusterOptions.
Additionally, a 'wal_dir' parameter is added to ExternalDaemonOptions to
separate the wal location from the data directories.

If 'num_data_dirs' is greater than 1, each daemon will generate multiple
paths, appending each with a numeric suffix, up to the number specified.

E.g. EMCs that would have used the path /cluster/data, if specifying
'num_data_dirs' as 3, will now generate multiple data directories
/cluster/data-0, /cluster/data-1, /cluster/data-2. The wal will be added
to /cluster/wal.

The new test multidir-cluster-itest demonstrates this.

This test has been run via dist-test 2000 with no flakes. Results here:
http://dist-test.cloudera.org/job?job_id=awong.1496701042.24862

Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/disk_reservation-itest.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.h
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_migration-itest.cc
A src/kudu/integration-tests/multidir_cluster-itest.cc
M src/kudu/integration-tests/open-readonly-fs-itest.cc
M src/kudu/integration-tests/ts_itest-base.h
M src/kudu/integration-tests/ts_recovery-itest.cc
15 files changed, 231 insertions(+), 61 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 24
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] external minicluster: expand EMC dir usage

[Andrew Wong (Code Review)]

Hello Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: external minicluster: expand EMC dir usage
..

external minicluster: expand EMC dir usage

In order to test different disk configurations, it is becoming
increasingly important to have end-to-end testing with nodes backed
by multiple directories.

External miniclusters by default use a single directory for each
daemon's data (i.e. wals and data dirs fall under a single
/cluster/daemon/data directory). This patch adds multi-directory support
via a new 'num_data_dirs' parameter to ExternalMiniClusterOptions.
Additionally, a 'wal_dir' parameter is added to ExternalDaemonOptions to
separate the wal location from the data directories.

If 'num_data_dirs' is greater than 1, each daemon will generate multiple
paths, appending each with a numeric suffix, up to the number specified.

E.g. EMCs that would have used the path /cluster/data, if specifying
'num_data_dirs' as 3, will now generate multiple data directories
/cluster/data-0, /cluster/data-1, /cluster/data-2. The wal will be added
to /cluster/wal.

The new test multidir-cluster-itest demonstrates this.

This test has been run via dist-test 2000 with no flakes. Results here:
http://dist-test.cloudera.org/job?job_id=awong.1496426200.3585

Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/disk_reservation-itest.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.h
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_migration-itest.cc
A src/kudu/integration-tests/multidir_cluster-itest.cc
M src/kudu/integration-tests/open-readonly-fs-itest.cc
M src/kudu/integration-tests/ts_itest-base.h
M src/kudu/integration-tests/ts_recovery-itest.cc
15 files changed, 231 insertions(+), 61 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 23
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] external minicluster: expand EMC dir usage

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: external minicluster: expand EMC dir usage
..


Patch Set 21:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6845/21/src/kudu/integration-tests/multidir_cluster-itest.cc
File src/kudu/integration-tests/multidir_cluster-itest.cc:

Line 89:   ASSERT_EVENTUALLY([&] {
Sorry, I meant to put this into the above ASSERT_EVENTUALLY block. Let's either 
do that or just leave it the way it was, since as you say once data is on disk 
we are guaranteed data is in the WAL.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 21
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] external minicluster: expand EMC dir usage

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: external minicluster: expand EMC dir usage
..


Patch Set 19:

(7 comments)

Retriggered after rebasing to master for tsan build.

http://gerrit.cloudera.org:8080/#/c/6845/19/src/kudu/integration-tests/CMakeLists.txt
File src/kudu/integration-tests/CMakeLists.txt:

Line 78: ADD_KUDU_TEST(multidir-cluster-itest)
> Nit: should come after master-stress-test, and should probably be called mu
Done


http://gerrit.cloudera.org:8080/#/c/6845/19/src/kudu/integration-tests/external_mini_cluster.cc
File src/kudu/integration-tests/external_mini_cluster.cc:

Line 255: CHECK_LE(*dir_index, opts_.num_data_dirs);
> Shouldn't this be CHECK_LT? If num_data_dirs=3, the only allowable values f
Done


Line 269: paths.push_back(GetDataPath(daemon_id, dir_index));
> Nit: use emplace_back() here.
Done


http://gerrit.cloudera.org:8080/#/c/6845/19/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:

Line 21: #include 
> https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includ
Yeah thinking about it wrt system-owned vs project-owned makes sense


Line 21: #include 
> Nit: this belongs in a separate group after the STL includes, because boost
Done


Line 300:   boost::optional dir_index = 
boost::none) const;
> Any particular reason we're using uint32_t for this stuff and not 'int'?
Mainly because I'd like to keep these indices non-negative. Could accomplish 
this with some checks, and that would allow us to avoid boost altogether, but 
using uint32_t makes it more explicit.


Line 428: DCHECK_EQ(1, data_dirs_.size());
> Nit: Should probably be CHECK_EQ() given that the other checks you've added
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 19
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] external minicluster: expand EMC dir usage

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: external minicluster: expand EMC dir usage
..


Patch Set 19:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6845/19/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:

Line 21: #include 
> I don't think we've standardized on that and I don't really care what we de
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

Seems pretty clear-cut to me: Boost is "Other libraries' .h files", not "C++ 
system files". I view the fact our thirdparty dependencies are placed on the 
"system" include path (and thus use triangular brackets instead of double 
quotes) as an implementation detail.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 19
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] external minicluster: expand EMC dir usage

[Andrew Wong (Code Review)]

Hello Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: external minicluster: expand EMC dir usage
..

external minicluster: expand EMC dir usage

In order to test different disk configurations, it is becoming
increasingly important to have end-to-end testing with nodes backed
by multiple directories.

External miniclusters by default use a single directory for each
daemon's data (i.e. wals and data dirs fall under a single
/cluster/daemon/data directory). This patch adds multi-directory support
via a new 'num_data_dirs' parameter to ExternalMiniClusterOptions.
Additionally, a 'wal_dir' parameter is added to ExternalDaemonOptions to
separate the wal location from the data directories.

If 'num_data_dirs' is greater than 1, each daemon will generate multiple
paths, appending each with a numeric suffix, up to the number specified.

E.g. EMCs that would have used the path /cluster/data, if specifying
'num_data_dirs' as 3, will now generate multiple data directories
/cluster/data-0, /cluster/data-1, /cluster/data-2. The wal will be added
to /cluster/wal.

The new test multidir-cluster-itest demonstrates this.

This test has been run via dist-test 1000 with no flakes. Results here:
http://dist-test.cloudera.org/job?job_id=awong.1496426200.3585

Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/disk_reservation-itest.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.h
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_migration-itest.cc
A src/kudu/integration-tests/multidir_cluster-itest.cc
M src/kudu/integration-tests/open-readonly-fs-itest.cc
M src/kudu/integration-tests/ts_itest-base.h
M src/kudu/integration-tests/ts_recovery-itest.cc
15 files changed, 235 insertions(+), 61 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 21
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] external minicluster: expand EMC dir usage

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: external minicluster: expand EMC dir usage
..


Patch Set 19:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6845/19/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:

Line 21: #include 
> Nit: this belongs in a separate group after the STL includes, because boost
I don't think we've standardized on that and I don't really care what we decide 
on but it would be nice to agree on this (admittedly minor) point. I'm fine 
with doing it the way you suggest.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 19
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] external minicluster: expand EMC dir usage

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: external minicluster: expand EMC dir usage
..


Patch Set 19:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6845/19/src/kudu/integration-tests/multidir-cluster-itest.cc
File src/kudu/integration-tests/multidir-cluster-itest.cc:

PS19, Line 88:   // Check that WALs are being placed in the expected wal 
directory.
 :   vector wal_files;
 :   
ASSERT_OK(inspect_->ListFilesInDir(JoinPathSegments(ts->wal_dir(), "wals"), 
_files));
 :   ASSERT_FALSE(wal_files.empty());
> Hrm, I'm trying to think of whether we can have a written blocks without ha
Yeah you are right, it shouldn't be possible.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 19
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] external minicluster: expand EMC dir usage

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: external minicluster: expand EMC dir usage
..


Patch Set 19:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/6845/19/src/kudu/integration-tests/CMakeLists.txt
File src/kudu/integration-tests/CMakeLists.txt:

Line 78: ADD_KUDU_TEST(multidir-cluster-itest)
Nit: should come after master-stress-test, and should probably be called 
multidir_cluster-itest (dashed suffixes are reserved for kinds of tests).


http://gerrit.cloudera.org:8080/#/c/6845/19/src/kudu/integration-tests/external_mini_cluster.cc
File src/kudu/integration-tests/external_mini_cluster.cc:

Line 255: CHECK_LE(*dir_index, opts_.num_data_dirs);
Shouldn't this be CHECK_LT? If num_data_dirs=3, the only allowable values for 
dir_index should be 0, 1, and 2, right?


Line 269: paths.push_back(GetDataPath(daemon_id, dir_index));
Nit: use emplace_back() here.


http://gerrit.cloudera.org:8080/#/c/6845/19/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:

Line 21: #include 
Nit: this belongs in a separate group after the STL includes, because boost is 
part of the "Kudu project".


Line 300:   boost::optional dir_index = 
boost::none) const;
Any particular reason we're using uint32_t for this stuff and not 'int'?


Line 428: DCHECK_EQ(1, data_dirs_.size());
Nit: Should probably be CHECK_EQ() given that the other checks you've added are 
not debug-only.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 19
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] external minicluster: expand EMC dir usage

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: external minicluster: expand EMC dir usage
..


Patch Set 17:

(5 comments)

Yep, added the results to the commit message

http://gerrit.cloudera.org:8080/#/c/6845/19/src/kudu/integration-tests/disk_reservation-itest.cc
File src/kudu/integration-tests/disk_reservation-itest.cc:

Line 68:   NO_FATALS(StartClusterWithOpts(opts));
> Looks like you can use this now:
Done


http://gerrit.cloudera.org:8080/#/c/6845/19/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:

Line 422:   // Delete files specified by 'wal_dir_' and 'data_dirs_'.
> add: WARN_UNUSED_RESULT
Done


http://gerrit.cloudera.org:8080/#/c/6845/19/src/kudu/integration-tests/master_failover-itest.cc
File src/kudu/integration-tests/master_failover-itest.cc:

Line 366: failed_master->DeleteFromDisk();
> ASSERT_OK()
Done


http://gerrit.cloudera.org:8080/#/c/6845/19/src/kudu/integration-tests/multidir-cluster-itest.cc
File src/kudu/integration-tests/multidir-cluster-itest.cc:

Line 49: "--fs_target_data_dirs_per_tablet=0"
> NO_FATALS()
Done


PS19, Line 88:   });
 :   work.StopAndJoin();
 : 
 :   // Check that WALs are being pla
> Come to think of it, it wouldn't hurt to put this inside the ASSERT_EVENTUA
Hrm, I'm trying to think of whether we can have a written blocks without having 
written to wal. I don't think so, but agreed, wouldn't hurt


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] external minicluster: expand EMC dir usage

[Andrew Wong (Code Review)]

Hello Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: external minicluster: expand EMC dir usage
..

external minicluster: expand EMC dir usage

In order to test different disk configurations, it is becoming
increasingly important to have end-to-end testing with nodes backed
by multiple directories.

External miniclusters by default use a single directory for each
daemon's data (i.e. wals and data dirs fall under a single
/cluster/daemon/data directory). This patch adds multi-directory support
via a new 'num_data_dirs' parameter to ExternalMiniClusterOptions.
Additionally, a 'wal_dir' parameter is added to ExternalDaemonOptions to
separate the wal location from the data directories.

If 'num_data_dirs' is greater than 1, each daemon will generate multiple
paths, appending each with a numeric suffix, up to the number specified.

E.g. EMCs that would have used the path /cluster/data, if specifying
'num_data_dirs' as 3, will now generate multiple data directories
/cluster/data-0, /cluster/data-1, /cluster/data-2. The wal will be added
to /cluster/wal.

The new test multidir-cluster-itest demonstrates this.

This test has been run via dist-test 1000 with no flakes. Results here:
http://dist-test.cloudera.org/job?job_id=awong.1496426200.3585

Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/disk_reservation-itest.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.h
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_migration-itest.cc
A src/kudu/integration-tests/multidir-cluster-itest.cc
M src/kudu/integration-tests/open-readonly-fs-itest.cc
M src/kudu/integration-tests/ts_itest-base.h
M src/kudu/integration-tests/ts_recovery-itest.cc
15 files changed, 234 insertions(+), 61 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 20
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] external minicluster: expand EMC dir usage

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: external minicluster: expand EMC dir usage
..


Patch Set 19:

(5 comments)

I don't think the test should be flaky anymore but did you try a few dist-test 
loops?

http://gerrit.cloudera.org:8080/#/c/6845/19/src/kudu/integration-tests/disk_reservation-itest.cc
File src/kudu/integration-tests/disk_reservation-itest.cc:

Line 68:   NO_FATALS(StartClusterWithOpts(opts));
Looks like you can use this now:

  NO_FATALS(StartCluster(ts_flags, {}, /* num_tablet_servers= */ 1, /* 
num_data_dirs= */ 2));


http://gerrit.cloudera.org:8080/#/c/6845/19/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:

Line 422:   // Delete files specified by 'wal_dir_' and 'data_dirs_'.
add: WARN_UNUSED_RESULT


http://gerrit.cloudera.org:8080/#/c/6845/19/src/kudu/integration-tests/master_failover-itest.cc
File src/kudu/integration-tests/master_failover-itest.cc:

Line 366: failed_master->DeleteFromDisk();
ASSERT_OK()


http://gerrit.cloudera.org:8080/#/c/6845/19/src/kudu/integration-tests/multidir-cluster-itest.cc
File src/kudu/integration-tests/multidir-cluster-itest.cc:

Line 49:   StartCluster(ts_flags, {}, /* num_tablet_servers */ 1, kNumDataDirs);
NO_FATALS()


PS19, Line 88:   // Check that WALs are being placed in the expected wal 
directory.
 :   vector wal_files;
 :   
ASSERT_OK(inspect_->ListFilesInDir(JoinPathSegments(ts->wal_dir(), "wals"), 
_files));
 :   ASSERT_FALSE(wal_files.empty());
Come to think of it, it wouldn't hurt to put this inside the 
ASSERT_EVENTUALLY() as well.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 19
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] external minicluster: expand EMC dir usage

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: external minicluster: expand EMC dir usage
..


Patch Set 19: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 19
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] external minicluster: expand EMC dir usage

[Andrew Wong (Code Review)]

Hello Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: external minicluster: expand EMC dir usage
..

external minicluster: expand EMC dir usage

In order to test different disk configurations, it is becoming
increasingly important to have end-to-end testing with nodes backed
by multiple directories.

External miniclusters by default use a single directory for each
daemon's data (i.e. wals and data dirs fall under a single
/cluster/daemon/data directory). This patch adds multi-directory support
via a new 'num_data_dirs' parameter to ExternalMiniClusterOptions.
Additionally, a 'wal_dir' parameter is added to ExternalDaemonOptions to
separate the wal location from the data directories.

If 'num_data_dirs' is greater than 1, each daemon will generate multiple
paths, appending each with a numeric suffix, up to the number specified.

E.g. EMCs that would have used the path /cluster/data, if specifying
'num_data_dirs' as 3, will now generate multiple data directories
/cluster/data-0, /cluster/data-1, /cluster/data-2. The wal will be added
to /cluster/wal.

The new test multidir-cluster-itest demonstrates this.

This test has been run via dist-test 1000 with no flakes. Results here:
http://dist-test.cloudera.org/job?job_id=awong.1496426200.3585

Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/disk_reservation-itest.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.h
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_migration-itest.cc
A src/kudu/integration-tests/multidir-cluster-itest.cc
M src/kudu/integration-tests/open-readonly-fs-itest.cc
M src/kudu/integration-tests/ts_itest-base.h
M src/kudu/integration-tests/ts_recovery-itest.cc
15 files changed, 237 insertions(+), 61 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 19
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] external minicluster: expand EMC dir usage

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: external minicluster: expand EMC dir usage
..


Patch Set 17:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6845/17/src/kudu/integration-tests/ts_recovery-itest.cc
File src/kudu/integration-tests/ts_recovery-itest.cc:

PS17, Line 328: opts.read_only = false;
  : opts.metric_entity = nullptr;
> aren't these defaults?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] external minicluster: expand EMC dir usage

[Andrew Wong (Code Review)]

Hello Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: external minicluster: expand EMC dir usage
..

external minicluster: expand EMC dir usage

In order to test different disk configurations, it is becoming
increasingly important to have end-to-end testing with nodes backed
by multiple directories.

External miniclusters by default use a single directory for each
daemon's data (i.e. wals and data dirs fall under a single
/cluster/daemon/data directory). This patch adds multi-directory support
via a new 'num_data_dirs' parameter to ExternalMiniClusterOptions.
Additionally, a 'wal_dir' parameter is added to ExternalDaemonOptions to
separate the wal location from the data directories.

If 'num_data_dirs' is greater than 1, each daemon will generate multiple
paths, appending each with a numeric suffix, up to the number specified.

E.g. EMCs that would have used the path /cluster/data, if specifying
'num_data_dirs' as 3, will now generate multiple data directories
/cluster/data-0, /cluster/data-1, /cluster/data-2. The wal will be added
to /cluster/wal.

The new test multidir-cluster-itest demonstrates this.

This test has been run via dist-test 1000 with no flakes. Results here:
http://dist-test.cloudera.org/job?job_id=awong.1496426200.3585

Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/disk_reservation-itest.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.h
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_migration-itest.cc
A src/kudu/integration-tests/multidir-cluster-itest.cc
M src/kudu/integration-tests/open-readonly-fs-itest.cc
M src/kudu/integration-tests/ts_itest-base.h
M src/kudu/integration-tests/ts_recovery-itest.cc
15 files changed, 239 insertions(+), 61 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] external minicluster: expand EMC dir usage

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: external minicluster: expand EMC dir usage
..


Patch Set 17:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/6845/16/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:

Line 105:   ~FsManager();
> why have this instead of just passing in an FsManagerOpts with the appropri
Yeah good point, this is only used in one place right now. Done


http://gerrit.cloudera.org:8080/#/c/6845/16/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:

Line 426: 
> worth DCHECKing here that data_dirs_.size() == 1? seems like it might be ea
Done


http://gerrit.cloudera.org:8080/#/c/6845/16/src/kudu/integration-tests/multidir-cluster-itest.cc
File src/kudu/integration-tests/multidir-cluster-itest.cc:

Line 25: #include "kudu/gutil/map-util.h"
> nit: alpha order
Done


PS16, Line 40: 
> Would you mind using ExternalMiniClusterITestBase for this test? It's what 
Yeah, Alexey made this point too, I was originally planning on extending this 
test for disk failure scenarios. Having added more disk failure tests, it 
probably makes sense to keep disk failure separate. Done


Line 45: "--flush_threshold_mb=1",
> maybe TestWorkload could do this just as well?
Done


Line 83:   }
> You can use TestWorkload.Setup() to easily create a table
Done


Line 103
> You should be able to get the same effect using TestWorkload.Start() and St
Done


Line 104
> This is potentially flaky. See below for a suggestion.
Done


PS16, Line 107: 
  : 
  : 
  : 
  : 
  : 
  : 
  : 
  : 
  : 
  : 
  : 
  : 
  : 
> You can wrap this in ASSERT_EVENTUALLY() and avoid the potentially-flaky Sl
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] external minicluster: expand EMC dir usage

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: external minicluster: expand EMC dir usage
..

external minicluster: expand EMC dir usage

In order to test different disk configurations, it is becoming
increasingly important to have end-to-end testing with nodes backed
by multiple directories.

External miniclusters by default use a single directory for each
daemon's data (i.e. wals and data dirs fall under a single
/cluster/daemon/data directory). This patch adds multi-directory support
via a new 'num_data_dirs' parameter to ExternalMiniClusterOptions.
Additionally, a 'wal_dir' parameter is added to ExternalDaemonOptions to
separate the wal location from the data directories.

If 'num_data_dirs' is greater than 1, each daemon will generate multiple
paths, appending each with a numeric suffix, up to the number specified.

E.g. EMCs that would have used the path /cluster/data, if specifying
'num_data_dirs' as 3, will now generate multiple data directories
/cluster/data-0, /cluster/data-1, /cluster/data-2. The wal will be added
to /cluster/wal.

The new test multidir-cluster-itest demonstrates this.

This test has been run via dist-test 1000 with no flakes. Results here:
http://dist-test.cloudera.org/job?job_id=awong.1496426200.3585

Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/disk_reservation-itest.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.h
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_migration-itest.cc
A src/kudu/integration-tests/multidir-cluster-itest.cc
M src/kudu/integration-tests/open-readonly-fs-itest.cc
M src/kudu/integration-tests/ts_itest-base.h
M src/kudu/integration-tests/ts_recovery-itest.cc
15 files changed, 242 insertions(+), 61 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] external minicluster: expand EMC dir usage

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: external minicluster: expand EMC dir usage
..


Patch Set 16:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6845/16/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:

Line 105:   FsManager(Env* env, std::string wal_dir, std::vector 
data_dirs);
why have this instead of just passing in an FsManagerOpts with the appropriate 
settings?


http://gerrit.cloudera.org:8080/#/c/6845/16/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:

Line 426:   const std::string& data_dir() const { return data_dirs_[0]; }
worth DCHECKing here that data_dirs_.size() == 1? seems like it might be easy 
for a test author to waste some time here if they made this mistake


http://gerrit.cloudera.org:8080/#/c/6845/16/src/kudu/integration-tests/multidir-cluster-itest.cc
File src/kudu/integration-tests/multidir-cluster-itest.cc:

Line 45: void MultiDirClusterITest::InsertPayload(const string& table_name, int 
start_row,
maybe TestWorkload could do this just as well?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] external minicluster: expand EMC dir usage

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: external minicluster: expand EMC dir usage
..


Patch Set 16:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/6845/16/src/kudu/integration-tests/multidir-cluster-itest.cc
File src/kudu/integration-tests/multidir-cluster-itest.cc:

Line 25: #include "kudu/integration-tests/ts_itest-base.h"
nit: alpha order


PS16, Line 40: TabletServerIntegrationTestBase
Would you mind using ExternalMiniClusterITestBase for this test? It's what 
we've been standardizing on for more recent itests.


Line 83:   NO_FATALS(CreateTable());
You can use TestWorkload.Setup() to easily create a table


Line 103:   InsertPayload(kTableId, 0, 10, 10);
You should be able to get the same effect using TestWorkload.Start() and 
StopAndJoin() after the ASSERT_EVENTUALLY


Line 104:   SleepFor(kTimeout);
This is potentially flaky. See below for a suggestion.

After making those changes, would you mind running 200 iterations of this test 
using dist_test.py with --stress_cpu_threads=8 and post the link to the 
results? LMK if you need help to get that set up and I can show you how to do 
it.


PS16, Line 107:   int num_dirs_added_to = 0;
  :   for (const string& data_dir : ext_tserver->data_dirs()) {
  : string data_path = JoinPathSegments(data_dir, "data");
  : vector files;
  : inspect_->ListFilesInDir(data_path, );
  : int* num_files_before_insert = 
FindOrNull(num_files_in_each_dir, data_dir);
  : ASSERT_NE(nullptr, num_files_before_insert);
  : if (*num_files_before_insert < files.size()) {
  :   num_dirs_added_to++;
  : }
  :   }
  :   // Block placement should guarantee that more than one data 
dir will have
  :   // data written to it.
  :   ASSERT_GT(num_dirs_added_to, 1);
You can wrap this in ASSERT_EVENTUALLY() and avoid the potentially-flaky 
SleepFor() above


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] external minicluster: expand EMC dir usage

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: external minicluster: expand EMC dir usage
..


Patch Set 16:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/6845/15/src/kudu/integration-tests/delete_table-itest.cc
File src/kudu/integration-tests/delete_table-itest.cc:

Line 1063:   ASSERT_OK(cluster_->master()->DeleteFromDisk());
> Would it make sense to just provide a DeleteFromDisk() method on the daemon
Done


http://gerrit.cloudera.org:8080/#/c/6845/15/src/kudu/integration-tests/external_mini_cluster.cc
File src/kudu/integration-tests/external_mini_cluster.cc:

Line 253:   string data_path = "data";
> I think we should add a check s.t. if dir_index is not passed then we must 
Done


PS15, Line 254: if (dir_index) {
> not a big deal but you can just say if (dir_index) here if you want
Done


PS15, Line 255: CHECK
> CHECK_LE
Done


PS15, Line 255: LE(*dir_index, 
> it's up to you but fyi you can also use the syntax *dir_index
Done


http://gerrit.cloudera.org:8080/#/c/6845/15/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:

Line 20: 
> nit: boost/optional isn't a standard C header, it should either go with the
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] external minicluster: expand EMC dir usage

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: external minicluster: expand EMC dir usage
..

external minicluster: expand EMC dir usage

In order to test different disk configurations, it is becoming
increasingly important to have end-to-end testing with nodes backed
by multiple directories.

External miniclusters by default use a single directory for each
daemon's data (i.e. wals and data dirs fall under a single
/cluster/daemon/data directory). This patch adds multi-directory support
via a new 'num_data_dirs' parameter to ExternalMiniClusterOptions.
Additionally, a 'wal_dir' parameter is added to ExternalDaemonOptions to
separate the wal location from the data directories.

If 'num_data_dirs' is greater than 1, each daemon will generate multiple
paths, appending each with a numeric suffix, up to the number specified.

E.g. EMCs that would have used the path /cluster/data, if specifying
'num_data_dirs' as 3, will now generate multiple data directories
/cluster/data-0, /cluster/data-1, /cluster/data-2. The wal will be added
to /cluster/wal.

The new test multidir-cluster-itest demonstrates this.

Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
---
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/disk_reservation-itest.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_migration-itest.cc
A src/kudu/integration-tests/multidir-cluster-itest.cc
M src/kudu/integration-tests/open-readonly-fs-itest.cc
M src/kudu/integration-tests/ts_itest-base.h
M src/kudu/integration-tests/ts_recovery-itest.cc
15 files changed, 271 insertions(+), 63 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] external minicluster: expand EMC dir usage

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: external minicluster: expand EMC dir usage
..


Patch Set 15:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/6845/15/src/kudu/integration-tests/delete_table-itest.cc
File src/kudu/integration-tests/delete_table-itest.cc:

Line 1063:   ASSERT_OK(env_->DeleteRecursively(cluster_->master()->data_dir()));
Would it make sense to just provide a DeleteFromDisk() method on the daemon?


http://gerrit.cloudera.org:8080/#/c/6845/15/src/kudu/integration-tests/external_mini_cluster.cc
File src/kudu/integration-tests/external_mini_cluster.cc:

Line 253:   string data_path = "data";
I think we should add a check s.t. if dir_index is not passed then we must have 
only one data dir (and doc that requirement in the header file):

  if (!dir_index) {
CHECK_EQ(1, opts_.num_data_dirs);
  }


PS15, Line 254: if (dir_index != boost::none)
not a big deal but you can just say if (dir_index) here if you want


PS15, Line 255: dir_index.get()
it's up to you but fyi you can also use the syntax *dir_index


PS15, Line 255: CHECK
CHECK_LE


http://gerrit.cloudera.org:8080/#/c/6845/15/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:

Line 20: #include 
nit: boost/optional isn't a standard C header, it should either go with the C++ 
stdlib includes or with the thirdparty C++ includes


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes