You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2017/05/10 19:33:05 UTC
[kudu-CR] WIP Allow external miniclusters to use many data dirs
Andrew Wong has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6845
Change subject: WIP Allow external miniclusters to use many data dirs
......................................................................
WIP Allow external miniclusters to use many data dirs
In order to test different disk configurations, it is becoming
increasingly important to have end-to-end testing with nodes backed
by multiple directories.
This patch adds this functionality to the ExternalMiniCluster class,
which now supports the 'num_dirs_per_tserver' option, and the
ExternalDaemon class, which now supports a list of data dirs instead
of a single data dir.
The ExternalMiniCluster will create the ExternalDaemon with a list of
data dirs that reflects 'num_dirs_per_tserver' by using the original
data dir name as a prefix and appending an integer (e.g. '/dir_name'
with 'num_dirs_per_tserver = 2' will result in '/dir_name-0' and
'/dir_name-1').
A new test called disk-failure-itest is added that exercises this.
WIP: EIO-handling patch must be completed for further testing to make
sense.
Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/disk-failure-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/ts_itest-base.h
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
7 files changed, 266 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/6845/1
--
To view, visit http://gerrit.cloudera.org:8080/6845
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] minicluster: facilitate creating multidir clusters
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 (#10).
Change subject: minicluster: facilitate creating multidir clusters
......................................................................
minicluster: facilitate creating multidir clusters
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, data dirs, logs fall under a single
/cluster/daemon/data directory). This patch adds multi-directory via a
new optional ExternalDaemonPathSpec struct.
Since miniclusters generate directories under a single cluster root,
specifying full path names via additional flags is tedious to do at
runtime for multiple daemons, only made worse by the fact that
miniclusters also create default directories based on the daemon's path.
Instead, the ExternalDaemonPathSpec specifies subdirectories as a list
of numeric postfixes to add to the default path. On top of specifying
data dirs, the PathSpec also allows specification of the wal and log
dirs.
E.g. miniclusters that would have used the path /cluster/data can
now specify multiple directories /cluster/data-1, /cluster/data-2,
/cluster/data-3, etc.
/test_path
+-cluster
+-daemon (master, ts-0, ts-1)
+-datadir (data-0, data-1, ...; before, this would be a single dir)
+-data, consensus-meta, etc.
The new test multidir-cluster-itest demonstrates this.
Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/multidir-cluster-itest.cc
M src/kudu/integration-tests/ts_itest-base.h
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
7 files changed, 232 insertions(+), 30 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/6845/10
--
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: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP Allow external miniclusters to use many data dirs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: WIP Allow external miniclusters to use many data dirs
......................................................................
Patch Set 1:
(1 comment)
Even the most cursory glance is greatly appreciated!
http://gerrit.cloudera.org:8080/#/c/6845/1/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:
Line 70: // Number of directories to create for each daemon.
> Wouldn't it be more natural to convert data_root into e.g. data_roots and a
Hmm, the ext cluster currently is organized as:
/test-path
+-cluster (this level is what is included in data_root)
+-daemon (master, ts-0, ts-1)
+- data dir (now supports multiple: data-0, data-1 via num_dirs_per_tserver)
+- wals, data, etc.
I suppose we could specify multiple directories for each cluster, have each directory mirror each other's layout, and spread data that way.
e.g.
/cluster-0/ts-0/data/{wals,data,etc},
/cluster-1/ts-0/data/data,
/cluster-2/ts-0/data/data
Where cluster-i is the ith disk on a cluster. Seems a bit complex, but good point that it could be used to test actual multi-disk systems. Is that what you're envisioning?
I could see replacing num_dirs_per_tserver with a list of path posfixes, although this would only serve as an aesthetic bump, if anything.
--
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: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <boost/optional.hpp>
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <boost/optional.hpp>
> 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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
Reviewed-by: Mike Percy <mp...@apache.org>
---
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] minicluster: facilitate creating multidir clusters
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: minicluster: facilitate creating multidir clusters
......................................................................
Patch Set 11:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/6845/9/src/kudu/integration-tests/external_mini_cluster.cc
File src/kudu/integration-tests/external_mini_cluster.cc:
PS9, Line 255:
> nit: not sure that's the best name for the parameter
Ah! Suffix. Will use that over postfix/nums here and elsewhere.
http://gerrit.cloudera.org:8080/#/c/6845/9/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:
PS9, Line 26: #include <vector>
> This is from the system C headers, this should come the very first in this
Done
http://gerrit.cloudera.org:8080/#/c/6845/9/src/kudu/integration-tests/multidisk-cluster-itest.cc
File src/kudu/integration-tests/multidisk-cluster-itest.cc:
Line 21
> nit: consider adding
Done
PS9, Line 26:
> I don't think it's necessary.
Done
PS9, Line 44:
> Why not just ExternalMiniClusterITestBase ? Is there anything specific y
I'm planning on using this test class to test other functionality for which some of the ts_itest utilities will be useful.
PS9, Line 46:
> I think you can drop this empty constructor -- it will be automatically gen
Done
PS9, Line 93:
> why not ASSERT_GT()?
Done.
PS9, Line 129:
> I'm confused by the combination of the comment and this assert: if I'm not
Agh, a very dumb mistake indeed. Operators were swapped.
http://gerrit.cloudera.org:8080/#/c/6845/9/src/kudu/util/path_util.cc
File src/kudu/util/path_util.cc:
PS9, Line 58: vector<string> segments;
: if (path[0] == '/') segments.push_back("/");
: vector<StringPiece> pieces = Split(path, "/", SkipEmpty());
: for (const StringPiece& piece : pieces) {
: segments.emplace_back(piece.data(), piece.size());
: }
: return segments;
: }
:
: string DirName(const string& path) {
:
> It seems using JoinStrings(paths, ",") could be a better fit here. If so,
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: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <boost/optional.hpp>
Nit: this belongs in a separate group after the STL includes, because boost is part of the "Kudu project".
Line 300: boost::optional<uint32_t> 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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] WIP Allow external miniclusters to use many data dirs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: WIP Allow external miniclusters to use many data dirs
......................................................................
Patch Set 1:
(1 comment)
I'll hold off on a full review until you're more comfortable with this and the compile/test failures have been addressed, but I did glance at the API.
http://gerrit.cloudera.org:8080/#/c/6845/1/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:
Line 70: // Number of directories to create for each daemon.
Wouldn't it be more natural to convert data_root into e.g. data_roots and allow multiple roots to be specified? That actually opens the door to running the mini cluster on a bunch of disks (provided the test code can collect the disk paths via gflag or some such).
--
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: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] minicluster: facilitate creating multidir clusters
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: minicluster: facilitate creating multidir clusters
......................................................................
Patch Set 13:
(2 comments)
In general I would like it to be possible to pass in some kind of options only once and then ignore them for the rest of the time using EMC.
http://gerrit.cloudera.org:8080/#/c/6845/13//COMMIT_MSG
Commit Message:
Line 7: minicluster: facilitate creating multidir clusters
external minicluster
http://gerrit.cloudera.org:8080/#/c/6845/13/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:
Line 61: struct ExternalDaemonPathSpec {
Can we make this part of ExternalMiniClusterOptions?
--
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: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] external minicluster: support multiple data dirs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 (#14).
Change subject: external minicluster: support multiple data dirs
......................................................................
external minicluster: support multiple data dirs
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/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/multidir-cluster-itest.cc
M src/kudu/integration-tests/ts_itest-base.h
5 files changed, 195 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/6845/14
--
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: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <boost/optional.hpp>
> 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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] WIP Allow external miniclusters to use many data dirs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 (#7).
Change subject: WIP Allow external miniclusters to use many data dirs
......................................................................
WIP Allow external miniclusters to use many data dirs
In order to test different disk configurations, it is becoming
increasingly important to have end-to-end testing with nodes backed
by multiple directories.
This patch adds this functionality to the ExternalMiniCluster class,
which now supports the 'num_dirs_per_tserver' option, and the
ExternalDaemon class, which now supports a list of data dirs instead
of a single data dir.
The ExternalMiniCluster will create the ExternalDaemon with a list of
data dirs that reflects 'num_dirs_per_tserver' by using the original
data dir name as a prefix and appending an integer (e.g. '/dir_name'
with 'num_dirs_per_tserver = 2' will result in '/dir_name-0' and
'/dir_name-1').
A new test called disk-failure-itest is added that exercises this.
WIP: EIO-handling patch must be completed for further testing to make
sense.
Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
---
M src/kudu/fs/file_block_manager.cc
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/disk-failure-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/ts_itest-base.h
M src/kudu/tablet/tablet_replica_mm_ops.cc
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
M src/kudu/util/status.h
10 files changed, 282 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/6845/7
--
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: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 (#15).
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, 259 insertions(+), 61 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/6845/15
--
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: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: external minicluster: expand EMC dir usage
......................................................................
Patch Set 17: Code-Review+1
(1 comment)
just one minor nit
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?
--
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] minicluster: facilitate creating multidir clusters
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: minicluster: facilitate creating multidir clusters
......................................................................
Patch Set 10:
(15 comments)
http://gerrit.cloudera.org:8080/#/c/6845/1/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:
Line 70: // Default: 1
> Yeah, that's more or less what I had in mind.
I agree that having multiple data roots could realistic configs (i.e. running a minicluster on a bunch of separate disks). What I'm not sold on is when we would actually want a test to do that. If we wanted to test for perf behavior differences running with some HDDs and some SSDs, for instance, I don't think a minicluster would be the right environment.
For now, I've added a bit to this API (even if it is just cosmetic) in hopes it'll be nicer to use.
http://gerrit.cloudera.org:8080/#/c/6845/9/src/kudu/integration-tests/multidisk-cluster-itest.cc
File src/kudu/integration-tests/multidisk-cluster-itest.cc:
Line 34
> warning: using decl 'Substitute' is unused [misc-unused-using-decls]
Done
Line 35
> warning: using decl 'KuduClient' is unused [misc-unused-using-decls]
Done
Line 36
> warning: using decl 'KuduSchema' is unused [misc-unused-using-decls]
Done
Line 39
> warning: using decl 'KuduTableCreator' is unused [misc-unused-using-decls]
Done
Line 40
> warning: using decl 'KuduSession' is unused [misc-unused-using-decls]
Done
Line 48
> warning: function 'kudu::MultiDiskClusterITest::InsertPayload' has a defini
Done
Line 56
> error: use of undeclared identifier 'shared_ptr' [clang-diagnostic-error]
Done
Line 56
> error: use of undeclared identifier 'session' [clang-diagnostic-error]
Done
Line 56
> error: 'KuduSession' does not refer to a value [clang-diagnostic-error]
Done
Line 57
> error: use of undeclared identifier 'session' [clang-diagnostic-error]
Done
Line 61
> error: no template named 'unique_ptr'; did you mean 'std::unique_ptr'? [cla
Done
Line 66
> error: use of undeclared identifier 'session' [clang-diagnostic-error]
Done
Line 67
> error: use of undeclared identifier 'session' [clang-diagnostic-error]
Done
http://gerrit.cloudera.org:8080/#/c/6845/9/src/kudu/integration-tests/ts_itest-base.h
File src/kudu/integration-tests/ts_itest-base.h:
Line 101: opts.tserver_path_spec = ts_path_spec;
> warning: parameter 'tserver_path_spec' is passed by value and only copied o
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: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] minicluster: facilitate creating multidir clusters
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 (#13).
Change subject: minicluster: facilitate creating multidir clusters
......................................................................
minicluster: facilitate creating multidir clusters
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 optional
ExternalDaemonPathSpec struct.
Since miniclusters generate directories under a single cluster root,
specifying full path names via additional flags is tedious to do at
runtime for multiple daemons, only made worse by the fact that
miniclusters also create default directories based on the daemon's path.
Instead, the ExternalDaemonPathSpec specifies subdirectories as a list
of numeric suffixes to add to the default path.
E.g. miniclusters that would have used the path /cluster/data can
now specify multiple directories /cluster/data-1, /cluster/data-2,
/cluster/data-3, etc.
/test_path
+-cluster
+-daemon (master, ts-0, ts-1)
+-datadir (data-0, data-1, ...; before, this would be a single dir)
+-data, consensus-meta, etc.
The new test multidir-cluster-itest demonstrates this.
Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/multidir-cluster-itest.cc
M src/kudu/integration-tests/ts_itest-base.h
5 files changed, 230 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/6845/13
--
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: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] minicluster: facilitate creating multidir clusters
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 (#11).
Change subject: minicluster: facilitate creating multidir clusters
......................................................................
minicluster: facilitate creating multidir clusters
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 optional
ExternalDaemonPathSpec struct.
Since miniclusters generate directories under a single cluster root,
specifying full path names via additional flags is tedious to do at
runtime for multiple daemons, only made worse by the fact that
miniclusters also create default directories based on the daemon's path.
Instead, the ExternalDaemonPathSpec specifies subdirectories as a list
of numeric suffixes to add to the default path.
E.g. miniclusters that would have used the path /cluster/data can
now specify multiple directories /cluster/data-1, /cluster/data-2,
/cluster/data-3, etc.
/test_path
+-cluster
+-daemon (master, ts-0, ts-1)
+-datadir (data-0, data-1, ...; before, this would be a single dir)
+-data, consensus-meta, etc.
The new test multidir-cluster-itest demonstrates this.
Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/multidir-cluster-itest.cc
M src/kudu/integration-tests/ts_itest-base.h
5 files changed, 228 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/6845/11
--
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: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] minicluster: facilitate creating multidir clusters
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: minicluster: facilitate creating multidir clusters
......................................................................
Patch Set 9:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/6845/9/src/kudu/integration-tests/external_mini_cluster.cc
File src/kudu/integration-tests/external_mini_cluster.cc:
PS9, Line 255: dir_nums
nit: not sure that's the best name for the parameter
what about 'dir_suffix' or alike?
http://gerrit.cloudera.org:8080/#/c/6845/9/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:
PS9, Line 26: #include <sys/types.h>
This is from the system C headers, this should come the very first in this file.
http://gerrit.cloudera.org:8080/#/c/6845/9/src/kudu/integration-tests/multidisk-cluster-itest.cc
File src/kudu/integration-tests/multidisk-cluster-itest.cc:
Line 21:
nit: consider adding
#include <gtest/gtest.h>
PS9, Line 26: #include "kudu/integration-tests/test_workload.h"
I don't think it's necessary.
PS9, Line 44: TabletServerIntegrationTestBase
Why not just ExternalMiniClusterITestBase ? Is there anything specific you want to bring from tablet-server specific test?
PS9, Line 46: MultiDiskClusterITest() {}
I think you can drop this empty constructor -- it will be automatically generated for you by the compiler.
PS9, Line 93: CHECK_GT
why not ASSERT_GT()?
What about just
ASSERT_FALSE(tablet_replicas_.empty());
PS9, Line 129: ASSERT_GT(1, num_dirs_added_to);
I'm confused by the combination of the comment and this assert: if I'm not mistaken, the latter is equivalent to ASSERT_TRUE(1 > num_dirs_added_to) and the former contradicts with that.
Consider either clarifying on the comment or updating the assert condition. Or I'm missing something :)
http://gerrit.cloudera.org:8080/#/c/6845/9/src/kudu/util/path_util.cc
File src/kudu/util/path_util.cc:
PS9, Line 58: string ListPaths(const vector<string>& paths) {
: if (paths.empty()) return "";
: std::stringstream ss;
: for (int path_num = 0; path_num < paths.size(); path_num++) {
: if (path_num != 0) {
: ss << ",";
: }
: ss << paths[path_num];
: }
: return ss.str();
: }
It seems using JoinStrings(paths, ",") could be a better fit here. If so, does it make sense to bring this new function at all?
If switching to JoinStrings, don't forget to remove the <sstream> include.
--
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: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <boost/optional.hpp>
> 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 <boost/optional.hpp>
> Nit: this belongs in a separate group after the STL includes, because boost
Done
Line 300: boost::optional<uint32_t> 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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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<string> wal_files;
: ASSERT_OK(inspect_->ListFilesInDir(JoinPathSegments(ts->wal_dir(), "wals"), &wal_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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] WIP Allow external miniclusters to use many data dirs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: WIP Allow external miniclusters to use many data dirs
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6845/1/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:
Line 70: // Number of directories to create for each daemon.
> Hmm, the ext cluster currently is organized as:
Yeah, that's more or less what I had in mind.
Agreed that the postfixes seem cosmetic at best; may not be worth the added code.
--
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: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] minicluster: more realistic multi-dir clusters
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 (#9).
Change subject: minicluster: more realistic multi-dir clusters
......................................................................
minicluster: more realistic multi-dir clusters
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, data dirs, logs fall under a single
/cluster/daemon/data directory). This patch adds multi-directory via a
new optional ExternalDaemonPathSpec struct.
Since miniclusters generate directories under a single cluster root,
specifying full path names via additional flags is tedious to do at
runtime for multiple tservers, only made worse by the fact that
miniclusters also create default directories based on the daemon's path.
Instead, the ExternalDaemonPathSpec specifies subdirectories as a list
of numeric postfixes to add to the default path. On top of specifying
data dirs, the PathSpec also allows specification of the wal and log
dirs.
E.g. miniclusters that would have used the path /cluster/data can
now specify multiple directories /cluster/data-1, /cluster/data-2,
/cluster/data-3, etc.
/test_path
+-cluster
+-daemon (master, ts-0, ts-1)
+-datadir (data-0, data-1, ...; before, this would be a single dir)
+-data, consensus-meta, etc.
The new test multidisk-cluster-itest demonstrates this.
Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/multidisk-cluster-itest.cc
M src/kudu/integration-tests/ts_itest-base.h
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
7 files changed, 236 insertions(+), 31 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/6845/9
--
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: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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<std::string> 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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] minicluster: facilitate creating multidir clusters
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: minicluster: facilitate creating multidir clusters
......................................................................
Patch Set 11:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/6845/11/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:
Line 218: const string& daemon_id, ExternalDaemonOptions* opts) const;
> error: unknown type name 'string'; did you mean 'std::string'? [clang-diagn
Done
Line 218: const string& daemon_id, ExternalDaemonOptions* opts) const;
> error: unknown type name 'string'; did you mean 'std::string'? [clang-diagn
Done
Line 218: const string& daemon_id, ExternalDaemonOptions* opts) const;
> error: unknown type name 'string'; did you mean 'std::string'? [clang-diagn
Done
http://gerrit.cloudera.org:8080/#/c/6845/11/src/kudu/integration-tests/multidir-cluster-itest.cc
File src/kudu/integration-tests/multidir-cluster-itest.cc:
Line 20: #include <string>
> warning: #includes are not sorted properly [llvm-include-order]
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: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Allow external miniclusters to use data dirs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 (#8).
Change subject: Allow external miniclusters to use data dirs
......................................................................
Allow external miniclusters to use data dirs
In order to test different disk configurations, it is becoming
increasingly important to have end-to-end testing with nodes backed
by multiple directories. Currently external miniclusters use a single
directory for each daemon's data (i.e. wals, data dirs, logs).
This patch adds this functionality to the ExternalMiniCluster class,
which now supports a new optional ExternalDaemonPathSpec struct.
Since miniclusters generate directories under a single data root,
specifying full path names isn't feasible at runtime. The
ExternalDaemonPathSpec specifies directories as a list of numeric
post-fixes to add to the original path.
E.g. mini clusters that would have used the path /mini/cluster/data
can now specify multiple directories /mini/cluster/data-1,
/mini/cluster/data-2, /mini/cluster/data-3, etc.
Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
---
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/ts_itest-base.h
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
5 files changed, 108 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/6845/8
--
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: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] WIP Allow external miniclusters to use many data dirs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 (#6).
Change subject: WIP Allow external miniclusters to use many data dirs
......................................................................
WIP Allow external miniclusters to use many data dirs
In order to test different disk configurations, it is becoming
increasingly important to have end-to-end testing with nodes backed
by multiple directories.
This patch adds this functionality to the ExternalMiniCluster class,
which now supports the 'num_dirs_per_tserver' option, and the
ExternalDaemon class, which now supports a list of data dirs instead
of a single data dir.
The ExternalMiniCluster will create the ExternalDaemon with a list of
data dirs that reflects 'num_dirs_per_tserver' by using the original
data dir name as a prefix and appending an integer (e.g. '/dir_name'
with 'num_dirs_per_tserver = 2' will result in '/dir_name-0' and
'/dir_name-1').
A new test called disk-failure-itest is added that exercises this.
WIP: EIO-handling patch must be completed for further testing to make
sense.
Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
---
M src/kudu/fs/file_block_manager.cc
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/disk-failure-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/ts_itest-base.h
M src/kudu/tablet/tablet_replica_mm_ops.cc
M src/kudu/util/env_posix.cc
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
M src/kudu/util/status.h
11 files changed, 286 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/6845/6
--
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: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] WIP Allow external miniclusters to use many data dirs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 (#4).
Change subject: WIP Allow external miniclusters to use many data dirs
......................................................................
WIP Allow external miniclusters to use many data dirs
In order to test different disk configurations, it is becoming
increasingly important to have end-to-end testing with nodes backed
by multiple directories.
This patch adds this functionality to the ExternalMiniCluster class,
which now supports the 'num_dirs_per_tserver' option, and the
ExternalDaemon class, which now supports a list of data dirs instead
of a single data dir.
The ExternalMiniCluster will create the ExternalDaemon with a list of
data dirs that reflects 'num_dirs_per_tserver' by using the original
data dir name as a prefix and appending an integer (e.g. '/dir_name'
with 'num_dirs_per_tserver = 2' will result in '/dir_name-0' and
'/dir_name-1').
A new test called disk-failure-itest is added that exercises this.
WIP: EIO-handling patch must be completed for further testing to make
sense.
Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/disk-failure-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/ts_itest-base.h
M src/kudu/util/env_posix.cc
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
8 files changed, 267 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/6845/4
--
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] minicluster: facilitate creating multidir clusters
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: minicluster: facilitate creating multidir clusters
......................................................................
Patch Set 12:
(1 comment)
build fail cause by lint check and a flake
http://gerrit.cloudera.org:8080/#/c/6845/12/src/kudu/integration-tests/external_mini_cluster.cc
File src/kudu/integration-tests/external_mini_cluster.cc:
Line 230: const string& daemon_id, ExternalDaemonOptions* opts) const{
space
--
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: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] minicluster: facilitate creating multidir clusters
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 (#12).
Change subject: minicluster: facilitate creating multidir clusters
......................................................................
minicluster: facilitate creating multidir clusters
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 optional
ExternalDaemonPathSpec struct.
Since miniclusters generate directories under a single cluster root,
specifying full path names via additional flags is tedious to do at
runtime for multiple daemons, only made worse by the fact that
miniclusters also create default directories based on the daemon's path.
Instead, the ExternalDaemonPathSpec specifies subdirectories as a list
of numeric suffixes to add to the default path.
E.g. miniclusters that would have used the path /cluster/data can
now specify multiple directories /cluster/data-1, /cluster/data-2,
/cluster/data-3, etc.
/test_path
+-cluster
+-daemon (master, ts-0, ts-1)
+-datadir (data-0, data-1, ...; before, this would be a single dir)
+-data, consensus-meta, etc.
The new test multidir-cluster-itest demonstrates this.
Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/multidir-cluster-itest.cc
M src/kudu/integration-tests/ts_itest-base.h
5 files changed, 230 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/6845/12
--
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: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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<string> files;
: inspect_->ListFilesInDir(data_path, &files);
: 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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] external minicluster: expand EMC dir usage
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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<string> wal_files;
: ASSERT_OK(inspect_->ListFilesInDir(JoinPathSegments(ts->wal_dir(), "wals"), &wal_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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes