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/06/16 19:43:33 UTC
[kudu-CR] mini server: support servers data dirs
Andrew Wong has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7211
Change subject: mini_server: support servers data dirs
......................................................................
mini_server: support servers data dirs
This patch gives MiniTabletServers the ability to start with multiple
data dirs. A number of dirs is specified and directory names are
generated for the specified number. The original directory will remain
as the WAL's directory.
E.g. if the number of data dirs is 3, the following directories will
be generated:
/test_dir/test_tserver_wal_dir
/test_dir/test_tserver_data_dir-0
/test_dir/test_tserver_data_dir-1
/test_dir/test_tserver_data_dir-2
instead of:
/test_dir/test_tserver
A test is added to tablet_server-test to exercise this.
Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
4 files changed, 32 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7211/1
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] mini cluster: 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/7211
to look at the new patch set (#8).
Change subject: mini_cluster: support multiple data dirs
......................................................................
mini_cluster: support multiple data dirs
This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. A WAL dir will
be generated with an appropriate suffix.
The setup for MiniMaster is also changed to more closely resemble
MiniTabletServer. MasterOptions are generated in the constructor and a
single Start() call will create the Master.
The original directory structure is kept as a default.
E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data0
/test_dir/test_tserver/data1
/test_dir/test_tserver/data2
Original:
/test_dir/test_tserver
Tests are added to tablet_server-test and master-test to exercise this.
Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
12 files changed, 127 insertions(+), 84 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7211/8
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] mini cluster: support multiple data dirs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: mini_cluster: support multiple data dirs
......................................................................
Patch Set 11:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/integration-tests/internal_mini_cluster.cc
File src/kudu/integration-tests/internal_mini_cluster.cc:
PS11, Line 60: fs_root_
> Can we get rid of this member too? We could put this in the body of the con
Done, but it came at the cost of making opts_ non-const.
http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/master/master-test.cc
File src/kudu/master/master-test.cc:
Line 1349: TEST_F(MasterTest, TestMultiDirMaster) {
> If this is just unit test coverage for the MiniMaster I think it should be
Done
http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
PS11, Line 59: data$0
> I think it would be more readable as "data-$0"
Done
Line 70: void MiniMaster::SetMasterAddresses(const vector<HostPort>& master_addrs) {
> nit: add CHECK(!master_) so this is only called when it's not running
Done
Line 71: opts_.master_addresses = master_addrs;
> nit: pass by value and use std::move() here
Done
PS11, Line 85: if (opts_.master_addresses.empty()) {
: return master_->WaitForCatalogManagerInit();
: }
> Add comment: // Wait for catalog manager to be ready if we only have a sing
Done
Line 92: opts_.rpc_opts.rpc_bind_addresses = bound_rpc_.ToString();
> add: CHECK(!master_) so this is only called after Shutdown()
Done
PS11, Line 93: HostPort(bound_http_).host()
> no need to construct a HostPort here, just call bound_http_.host() and get
Done
PS11, Line 94: HostPort(bound_http_).port()
> bound_http_.port()
Done
Line 95: Shutdown();
> Restart() has unusual semantics in the minicluster* classes. They require S
Ah I see, makes sense. I'll save that for another patch then; I don't have anything against those semantics.
To just keep the original behavior (with no Shutdown()), I'll just take out Shutdown() from Restart(). In all the tests, if Shutdown() is always called beforehand, the Shutdown() here would just be a no-op anyway.
http://gerrit.cloudera.org:8080/#/c/7211/12/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
Line 100: if (master_) {
> warning: an integer is interpreted as a character code when assigning it to
Done
http://gerrit.cloudera.org:8080/#/c/7211/12/src/kudu/tserver/mini_tablet_server-test.cc
File src/kudu/tserver/mini_tablet_server-test.cc:
Line 21
> warning: #includes are not sorted properly [llvm-include-order]
Done
http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:
PS11, Line 65: "data$0
> I know this is subjective but I think this would be a little more readable
Done
http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
Line 2486: TEST_F(TabletServerTest, TestMultiDirServer) {
> This should probably be in its own file if this is just a unit test for the
Done
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] mini cluster: support multiple data dirs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: mini_cluster: support multiple data dirs
......................................................................
Patch Set 15: Code-Review+1
(4 comments)
Looks good, just a couple nits
http://gerrit.cloudera.org:8080/#/c/7211/15/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
Line 84: RETURN_NOT_OK(env_util::CreateDirIfMissing(Env::Default(), DirName(opts_.fs_opts.wal_path),
It seems counterintuitive that we implicitly treat opts_.fs_opts.wal_path as the fs_root dir. I think we should keep fs_root_ as a member.
This will also matter once we add support for multiple wal dirs so better to just leave it as it was now.
PS15, Line 85: &created
nit: indentation and also you can leave this bool out if you don't use it (it defaults to nullptr)
http://gerrit.cloudera.org:8080/#/c/7211/15/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:
Line 82: RETURN_NOT_OK(env_util::CreateDirIfMissing(Env::Default(), DirName(opts_.fs_opts.wal_path),
Seems better to have an fs_root_ member here than piggyback on wal_path as noted in the other file
Line 83: &created));
nit: bool is optional
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] mini tablet server: support multiple data dirs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: mini_tablet_server: support multiple data dirs
......................................................................
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7211/6//COMMIT_MSG
Commit Message:
Line 7: mini_tablet_server: support multiple data dirs
> I think this would be better defined in MiniClusterOptions and then plumbed
Agreed
PS6, Line 18: /test_dir/test_tserver_wal_dir
: /test_dir/test_tserver_data_dir-0
: /test_dir/test_tserver_data_dir-1
: /test_dir/test_tserver_data_dir-2
> Hmm, what do you think about:
+1 on this
http://gerrit.cloudera.org:8080/#/c/7211/6/src/kudu/tserver/tablet_server-test-base.h
File src/kudu/tserver/tablet_server-test-base.h:
PS6, Line 100: = 1
Don't use default arguments with virtual functions. They don't really work.
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 6
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-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] mini cluster: support multiple data dirs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged.
Change subject: mini_cluster: support multiple data dirs
......................................................................
mini_cluster: support multiple data dirs
This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. A WAL dir will
be generated with an appropriate suffix.
The setup for MiniMaster is also changed to more closely resemble
MiniTabletServer. MasterOptions are generated in the constructor and a
single Start() call will create the Master.
The original directory structure is kept as a default.
E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data-0
/test_dir/test_tserver/data-1
/test_dir/test_tserver/data-2
Original:
/test_dir/test_tserver
Tests are added to the new mini_tablet_server-test and mini_master-test
to exercise this.
Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Reviewed-on: http://gerrit.cloudera.org:8080/7211
Reviewed-by: Mike Percy <mp...@apache.org>
Tested-by: Mike Percy <mp...@apache.org>
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master-test.cc
A src/kudu/master/mini_master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/CMakeLists.txt
A src/kudu/tserver/mini_tablet_server-test.cc
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
16 files changed, 233 insertions(+), 122 deletions(-)
Approvals:
Mike Percy: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] mini tablet server: 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/7211
to look at the new patch set (#2).
Change subject: mini_tablet_server: support multiple data dirs
......................................................................
mini_tablet_server: support multiple data dirs
This patch gives MiniTabletServers the ability to start with multiple
data dirs. A number of dirs is specified and directory names are
generated for the specified number. Additionally, a WAL dir will be
generated with an appropriate suffix.
The original directory structure is kept as a default.
E.g. if the number of data dirs is 3, the following directories will
be generated:
/test_dir/test_tserver_wal_dir
/test_dir/test_tserver_data_dir-0
/test_dir/test_tserver_data_dir-1
/test_dir/test_tserver_data_dir-2
Original:
/test_dir/test_tserver
A test is added to tablet_server-test to exercise this.
Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
4 files changed, 32 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7211/2
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] mini cluster: 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/7211
to look at the new patch set (#11).
Change subject: mini_cluster: support multiple data dirs
......................................................................
mini_cluster: support multiple data dirs
This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. A WAL dir will
be generated with an appropriate suffix.
The setup for MiniMaster is also changed to more closely resemble
MiniTabletServer. MasterOptions are generated in the constructor and a
single Start() call will create the Master.
The original directory structure is kept as a default.
E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data0
/test_dir/test_tserver/data1
/test_dir/test_tserver/data2
Original:
/test_dir/test_tserver
Tests are added to tablet_server-test and master-test to exercise this.
Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
12 files changed, 141 insertions(+), 106 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7211/11
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] mini cluster: 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/7211
to look at the new patch set (#7).
Change subject: mini_cluster: support multiple data dirs
......................................................................
mini_cluster: support multiple data dirs
This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. Additionally,
a WAL dir will be generated with an appropriate suffix. Additionally,
these can both be specified by MiniClusterOptions.
The original directory structure is kept as a default.
E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data0
/test_dir/test_tserver/data1
/test_dir/test_tserver/data2
Original:
/test_dir/test_tserver
Tests are added to tablet_server-test and master-test to exercise this.
Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/master/master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
11 files changed, 84 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7211/7
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] mini cluster: support multiple data dirs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: mini_cluster: support multiple data dirs
......................................................................
Patch Set 11:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/integration-tests/internal_mini_cluster.cc
File src/kudu/integration-tests/internal_mini_cluster.cc:
PS11, Line 60: fs_root_
Can we get rid of this member too? We could put this in the body of the constructor and finally get rid of this last non-opts member:
if (opts_.data_root.empty()) {
opts_.data_root = JoinPathSegments(GetTestDataDirectory(), "minicluster-data");
}
http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/master/master-test.cc
File src/kudu/master/master-test.cc:
Line 1349: TEST_F(MasterTest, TestMultiDirMaster) {
If this is just unit test coverage for the MiniMaster I think it should be given its own test file, i.e. mini_master-test.cc
http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
Line 70: void MiniMaster::SetMasterAddresses(const vector<HostPort>& master_addrs) {
nit: add CHECK(!master_) so this is only called when it's not running
Line 71: opts_.master_addresses = master_addrs;
nit: pass by value and use std::move() here
PS11, Line 85: if (opts_.master_addresses.empty()) {
: return master_->WaitForCatalogManagerInit();
: }
Add comment: // Wait for catalog manager to be ready if we only have a single master.
Actually, we should probably make this if (opts_.master_addresses.size() <= 1)
Line 92: opts_.rpc_opts.rpc_bind_addresses = bound_rpc_.ToString();
add: CHECK(!master_) so this is only called after Shutdown()
See below comment for explanation of why Shutdown() has to be called before Restart()
PS11, Line 93: HostPort(bound_http_).host()
no need to construct a HostPort here, just call bound_http_.host() and get the host from the Sockaddr directly.
PS11, Line 94: HostPort(bound_http_).port()
bound_http_.port()
Line 95: Shutdown();
Restart() has unusual semantics in the minicluster* classes. They require Shutdown() to be called, then Restart() to be called. Keep this consistent for now; if you want to change those semantics later we can consider it in a separate patch across Internal and External Minicluster classes.
By the way, Shutdown() is what sets bound_rpc_ and bound_http_ so AFAICT this would not really work as written. The fact that it doesn't fail any tests probably just indicates a lack of test coverage on MiniMaster.
http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
Line 2486: TEST_F(TabletServerTest, TestMultiDirServer) {
This should probably be in its own file if this is just a unit test for the MiniTabletServer, i.e. mini_tablet_server-test.cc
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] mini cluster: support multiple data dirs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: mini_cluster: support multiple data dirs
......................................................................
Patch Set 14:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/integration-tests/internal_mini_cluster.cc
File src/kudu/integration-tests/internal_mini_cluster.cc:
PS11, Line 60: opts_(st
> great - seems alright to me
Done
http://gerrit.cloudera.org:8080/#/c/7211/13/src/kudu/integration-tests/internal_mini_cluster.cc
File src/kudu/integration-tests/internal_mini_cluster.cc:
Line 58: InternalMiniCluster::InternalMiniCluster(Env* env, InternalMiniClusterOptions options)
> warning: pass by value and use std::move [modernize-pass-by-value]
Done
Line 60: opts_(std::move(options)),
> warning: parameter 'options' is passed by value and only copied once; consi
Done
http://gerrit.cloudera.org:8080/#/c/7211/14/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
Line 60: CHECK_OK(env_util::CreateDirIfMissing(Env::Default(), fs_root_, &created));
> Are we sure this step is required now but not previously?
Yeah, we require that the parent directory of any data dirs exists at startup time
Line 102: Shutdown();
> Shutdown() still here in Restart()
Ah thanks
http://gerrit.cloudera.org:8080/#/c/7211/14/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:
Line 65: CHECK_OK(env_util::CreateDirIfMissing(Env::Default(), fs_root, &created));
> See comment in mini_master.cc
Done
http://gerrit.cloudera.org:8080/#/c/7211/14/src/kudu/tserver/tablet_server-test-base.h
File src/kudu/tserver/tablet_server-test-base.h:
Line 103: tablet_replica_.reset();
> Why is this needed now but not before?
If StartTabletServer is before the tablet_replica_ is reset (either this way or with ShutdownTablet()), the dtor for the server will fail a CHECK because there's still exists a reference to some refcounted data (the tablet_replica_). When the multidir test was in tablet_server-test, that's what happened, but it isn't the case anymore.
Alternatively, we can enforce that no server is running (like in mini_master) and add multidir support to ShutdownAndRebuildTablet() too, which is probably the right thing to do.
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] mini cluster: 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/7211
to look at the new patch set (#12).
Change subject: mini_cluster: support multiple data dirs
......................................................................
mini_cluster: support multiple data dirs
This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. A WAL dir will
be generated with an appropriate suffix.
The setup for MiniMaster is also changed to more closely resemble
MiniTabletServer. MasterOptions are generated in the constructor and a
single Start() call will create the Master.
The original directory structure is kept as a default.
E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data0
/test_dir/test_tserver/data1
/test_dir/test_tserver/data2
Original:
/test_dir/test_tserver
Tests are added to the new mini_tablet_server-test and mini_master-test
to exercise this.
Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master-test.cc
A src/kudu/master/mini_master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/CMakeLists.txt
A src/kudu/tserver/mini_tablet_server-test.cc
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
16 files changed, 223 insertions(+), 109 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7211/12
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] mini cluster: support multiple data dirs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: mini_cluster: support multiple data dirs
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7211/7/src/kudu/integration-tests/internal_mini_cluster.h
File src/kudu/integration-tests/internal_mini_cluster.h:
Line 201: const int num_data_dirs_;
Hmm, doesn't seem like we need this (or fs_root_) since we have a copy of opts in opts_.
http://gerrit.cloudera.org:8080/#/c/7211/7/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
Line 110:
Not your fault, but could you make 'opts' a class member and set it up in the constructor, so it's more like MiniTabletServer?
Doing so would open the door to a common MiniServer implementation whose constructor can initialize ServerBaseOptions for both MiniMaster and MiniTabletServer.
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] mini tablet server: support multiple data dirs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: mini_tablet_server: support multiple data dirs
......................................................................
Patch Set 6:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7211/6//COMMIT_MSG
Commit Message:
Line 7: mini_tablet_server: support multiple data dirs
I think this would be better defined in MiniClusterOptions and then plumbed into both masters and tservers. MiniClusterOptions (and ExternalMiniClusterOptions) is how we typically configure our clusters; it's unusual to work with MiniTabletServer directly.
PS6, Line 18: /test_dir/test_tserver_wal_dir
: /test_dir/test_tserver_data_dir-0
: /test_dir/test_tserver_data_dir-1
: /test_dir/test_tserver_data_dir-2
Hmm, what do you think about:
/test_dir/test_tserver/wal
/test_dir/test_tserver/data0
/test_dir/test_tserver/data1
/test_dir/test_tserver/data2
It's a little shorter, and it retains the property that fs_root itself exists as a directory, which can be useful (e.g. compute disk space usage recursively from fs_root).
http://gerrit.cloudera.org:8080/#/c/7211/6/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:
Line 59: std::vector<string> fs_data_dirs;
Don't need std:: prefix.
PS6, Line 60: uint32_t
int here
http://gerrit.cloudera.org:8080/#/c/7211/6/src/kudu/tserver/mini_tablet_server.h
File src/kudu/tserver/mini_tablet_server.h:
PS6, Line 43: uint32_t
An int would be fine here too.
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 6
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-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] mini cluster: support multiple data dirs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: mini_cluster: support multiple data dirs
......................................................................
Patch Set 16:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7211/15/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
Line 84: RETURN_NOT_OK(env_util::CreateDirIfMissing(Env::Default(), fs_root_));
> It seems counterintuitive that we implicitly treat opts_.fs_opts.wal_path a
Hrm, for multiple wal dirs, I'd think that they'd still fall within a single minimaster root, but I agree it feels weird getting the root this way. Done
PS15, Line 85: );
> nit: indentation and also you can leave this bool out if you don't use it (
Done
http://gerrit.cloudera.org:8080/#/c/7211/15/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:
Line 82: RETURN_NOT_OK(env_util::CreateDirIfMissing(Env::Default(), fs_root_));
> Seems better to have an fs_root_ member here than piggyback on wal_path as
Done
Line 83: unique_ptr<TabletServer> server(new TabletServer(opts_));
> nit: bool is optional
Done
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] mini cluster: support multiple data dirs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: mini_cluster: support multiple data dirs
......................................................................
Patch Set 15:
The failure seems unrelated
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] mini cluster: support multiple data dirs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7211
to look at the new patch set (#16).
Change subject: mini_cluster: support multiple data dirs
......................................................................
mini_cluster: support multiple data dirs
This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. A WAL dir will
be generated with an appropriate suffix.
The setup for MiniMaster is also changed to more closely resemble
MiniTabletServer. MasterOptions are generated in the constructor and a
single Start() call will create the Master.
The original directory structure is kept as a default.
E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data-0
/test_dir/test_tserver/data-1
/test_dir/test_tserver/data-2
Original:
/test_dir/test_tserver
Tests are added to the new mini_tablet_server-test and mini_master-test
to exercise this.
Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master-test.cc
A src/kudu/master/mini_master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/CMakeLists.txt
A src/kudu/tserver/mini_tablet_server-test.cc
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
16 files changed, 232 insertions(+), 121 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7211/16
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] mini cluster: support multiple data dirs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: mini_cluster: support multiple data dirs
......................................................................
Patch Set 11:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
PS11, Line 59: data$0
I think it would be more readable as "data-$0"
http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:
PS11, Line 65: "data$0
I know this is subjective but I think this would be a little more readable when doing an "ls" as "data-$0"
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] mini cluster: support multiple data dirs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: mini_cluster: support multiple data dirs
......................................................................
Patch Set 7:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/7211/6//COMMIT_MSG
Commit Message:
Line 7: mini_cluster: support multiple data dirs
> Agreed
Done
Line 7: mini_cluster: support multiple data dirs
> I think this would be better defined in MiniClusterOptions and then plumbed
Done
PS6, Line 18: be generated for tservers (a similar one is created for masters):
: /test_dir/test_tserver/wal
: /test_dir/test_tserver/data0
: /test_dir/test_tserver/data1
> Hmm, what do you think about:
Done
PS6, Line 18: be generated for tservers (a similar one is created for masters):
: /test_dir/test_tserver/wal
: /test_dir/test_tserver/data0
: /test_dir/test_tserver/data1
> +1 on this
Done
http://gerrit.cloudera.org:8080/#/c/7211/7/src/kudu/integration-tests/internal_mini_cluster.h
File src/kudu/integration-tests/internal_mini_cluster.h:
Line 201: const int num_data_dirs_;
> Hmm, doesn't seem like we need this (or fs_root_) since we have a copy of o
Interestingly we weren't copying opts_. I've removed it in favor of keeping these private members.
http://gerrit.cloudera.org:8080/#/c/7211/7/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
Line 110:
> Not your fault, but could you make 'opts' a class member and set it up in t
Done
http://gerrit.cloudera.org:8080/#/c/7211/6/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:
Line 59: opts_.webserver_opts.port = 0;
> Don't need std:: prefix.
Done
PS6, Line 60: _data_di
> int here
Done
http://gerrit.cloudera.org:8080/#/c/7211/7/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:
Line 21: #include <utility>
> warning: #includes are not sorted properly [llvm-include-order]
Done
Line 32: #include "kudu/util/path_util.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
http://gerrit.cloudera.org:8080/#/c/7211/6/src/kudu/tserver/mini_tablet_server.h
File src/kudu/tserver/mini_tablet_server.h:
PS6, Line 43:
> An int would be fine here too.
Done
http://gerrit.cloudera.org:8080/#/c/7211/6/src/kudu/tserver/tablet_server-test-base.h
File src/kudu/tserver/tablet_server-test-base.h:
PS6, Line 100:
> Don't use default arguments with virtual functions. They don't really work.
Done
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] mini cluster: 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/7211
to look at the new patch set (#15).
Change subject: mini_cluster: support multiple data dirs
......................................................................
mini_cluster: support multiple data dirs
This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. A WAL dir will
be generated with an appropriate suffix.
The setup for MiniMaster is also changed to more closely resemble
MiniTabletServer. MasterOptions are generated in the constructor and a
single Start() call will create the Master.
The original directory structure is kept as a default.
E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data-0
/test_dir/test_tserver/data-1
/test_dir/test_tserver/data-2
Original:
/test_dir/test_tserver
Tests are added to the new mini_tablet_server-test and mini_master-test
to exercise this.
Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master-test.cc
A src/kudu/master/mini_master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/CMakeLists.txt
A src/kudu/tserver/mini_tablet_server-test.cc
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
16 files changed, 232 insertions(+), 120 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7211/15
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] mini cluster: 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/7211
to look at the new patch set (#13).
Change subject: mini_cluster: support multiple data dirs
......................................................................
mini_cluster: support multiple data dirs
This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. A WAL dir will
be generated with an appropriate suffix.
The setup for MiniMaster is also changed to more closely resemble
MiniTabletServer. MasterOptions are generated in the constructor and a
single Start() call will create the Master.
The original directory structure is kept as a default.
E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data0
/test_dir/test_tserver/data1
/test_dir/test_tserver/data2
Original:
/test_dir/test_tserver
Tests are added to the new mini_tablet_server-test and mini_master-test
to exercise this.
Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master-test.cc
A src/kudu/master/mini_master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/CMakeLists.txt
A src/kudu/tserver/mini_tablet_server-test.cc
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
16 files changed, 226 insertions(+), 114 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7211/13
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] mini cluster: support multiple data dirs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: mini_cluster: support multiple data dirs
......................................................................
Patch Set 17:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7211/16/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
Line 46: MiniMaster::MiniMaster(string fs_root, HostPort rpc_bind_addr, int num_data_dirs)
> warning: pass by value and use std::move [modernize-pass-by-value]
Done
http://gerrit.cloudera.org:8080/#/c/7211/16/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:
Line 50: MiniTabletServer::MiniTabletServer(string fs_root,
> warning: pass by value and use std::move [modernize-pass-by-value]
Done
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] mini cluster: support multiple data dirs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7211
to look at the new patch set (#17).
Change subject: mini_cluster: support multiple data dirs
......................................................................
mini_cluster: support multiple data dirs
This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. A WAL dir will
be generated with an appropriate suffix.
The setup for MiniMaster is also changed to more closely resemble
MiniTabletServer. MasterOptions are generated in the constructor and a
single Start() call will create the Master.
The original directory structure is kept as a default.
E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data-0
/test_dir/test_tserver/data-1
/test_dir/test_tserver/data-2
Original:
/test_dir/test_tserver
Tests are added to the new mini_tablet_server-test and mini_master-test
to exercise this.
Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master-test.cc
A src/kudu/master/mini_master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/CMakeLists.txt
A src/kudu/tserver/mini_tablet_server-test.cc
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
16 files changed, 233 insertions(+), 122 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7211/17
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] mini cluster: support multiple data dirs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: mini_cluster: support multiple data dirs
......................................................................
Patch Set 9:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7211/9/src/kudu/integration-tests/internal_mini_cluster.cc
File src/kudu/integration-tests/internal_mini_cluster.cc:
Line 65: bind_mode_(options.bind_mode),
This looks like a real bug (and good fix) from commit e675873. Can you make sure Mike knows about it?
http://gerrit.cloudera.org:8080/#/c/7211/7/src/kudu/integration-tests/internal_mini_cluster.h
File src/kudu/integration-tests/internal_mini_cluster.h:
Line 201: const int num_data_dirs_;
> Interestingly we weren't copying opts_. I've removed it in favor of keeping
Why? Wouldn't it be simpler to copy opts_ and drop these individual members?
And with your current approach what's the point of keeping opts_ around if it's not used?
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] mini cluster: support multiple data dirs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: mini_cluster: support multiple data dirs
......................................................................
Patch Set 14:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/integration-tests/internal_mini_cluster.cc
File src/kudu/integration-tests/internal_mini_cluster.cc:
PS11, Line 60: opts_(st
> Done, but it came at the cost of making opts_ non-const.
great - seems alright to me
http://gerrit.cloudera.org:8080/#/c/7211/14/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
Line 60: CHECK_OK(env_util::CreateDirIfMissing(Env::Default(), fs_root_, &created));
Are we sure this step is required now but not previously?
If it is required now, perhaps this should be RETURN_NOT_OK() in Start() instead of a CHECK() in the constructor since it's not there to enforce a particular API, ensure correctness, or run on a non-main thread.
Line 102: Shutdown();
Shutdown() still here in Restart()
http://gerrit.cloudera.org:8080/#/c/7211/14/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:
Line 65: CHECK_OK(env_util::CreateDirIfMissing(Env::Default(), fs_root, &created));
See comment in mini_master.cc
http://gerrit.cloudera.org:8080/#/c/7211/14/src/kudu/tserver/tablet_server-test-base.h
File src/kudu/tserver/tablet_server-test-base.h:
Line 103: tablet_replica_.reset();
Why is this needed now but not before?
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] mini cluster: support multiple data dirs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: mini_cluster: support multiple data dirs
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7211/9/src/kudu/integration-tests/internal_mini_cluster.cc
File src/kudu/integration-tests/internal_mini_cluster.cc:
Line 65: master_rpc_ports_(options.master_rpc_ports),
> This looks like a real bug (and good fix) from commit e675873. Can you make
Will do
http://gerrit.cloudera.org:8080/#/c/7211/7/src/kudu/integration-tests/internal_mini_cluster.h
File src/kudu/integration-tests/internal_mini_cluster.h:
Line 201: const int num_data_dirs_;
> Why? Wouldn't it be simpler to copy opts_ and drop these individual members
Done
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] mini cluster: 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/7211
to look at the new patch set (#14).
Change subject: mini_cluster: support multiple data dirs
......................................................................
mini_cluster: support multiple data dirs
This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. A WAL dir will
be generated with an appropriate suffix.
The setup for MiniMaster is also changed to more closely resemble
MiniTabletServer. MasterOptions are generated in the constructor and a
single Start() call will create the Master.
The original directory structure is kept as a default.
E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data-0
/test_dir/test_tserver/data-1
/test_dir/test_tserver/data-2
Original:
/test_dir/test_tserver
Tests are added to the new mini_tablet_server-test and mini_master-test
to exercise this.
Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master-test.cc
A src/kudu/master/mini_master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/CMakeLists.txt
A src/kudu/tserver/mini_tablet_server-test.cc
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
16 files changed, 226 insertions(+), 114 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7211/14
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] mini cluster: support multiple data dirs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: mini_cluster: support multiple data dirs
......................................................................
Patch Set 17: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] mini cluster: support multiple data dirs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: mini_cluster: support multiple data dirs
......................................................................
Patch Set 17: Verified+1
Looks like a flaky java test, overriding jenkins
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] mini cluster: 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/7211
to look at the new patch set (#9).
Change subject: mini_cluster: support multiple data dirs
......................................................................
mini_cluster: support multiple data dirs
This patch gives MiniTabletServers and MiniMasters the ability to
start with multiple data dirs. A number of dirs is specified and
directory names are generated for the specified number. A WAL dir will
be generated with an appropriate suffix.
The setup for MiniMaster is also changed to more closely resemble
MiniTabletServer. MasterOptions are generated in the constructor and a
single Start() call will create the Master.
The original directory structure is kept as a default.
E.g. if the number of data dirs is 3, the following directories will
be generated for tservers (a similar one is created for masters):
/test_dir/test_tserver/wal
/test_dir/test_tserver/data0
/test_dir/test_tserver/data1
/test_dir/test_tserver/data2
Original:
/test_dir/test_tserver
Tests are added to tablet_server-test and master-test to exercise this.
Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
---
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
12 files changed, 126 insertions(+), 80 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7211/9
--
To view, visit http://gerrit.cloudera.org:8080/7211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot