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 2018/01/13 17:32:35 UTC
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9027
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
KUDU-1489: allow configuration of metadata dir
Metadata files currently reside in the first configured data directory,
which isn't always the best choice. This first drive, if not performant,
can act as a bottleneck; often times the drive containing the WALs is a
better choice, as it is recommended to be the fastest.
This patch allows users to configure their metadata directory through
the new fs_metadata_dir flag. If empty, Kudu will check if a metadata
directory exists in the first member of fs_data_dirs, and if none
exists, Kudu will use fs_wal_dir as the root directory for metadata.
Otherwise, the flag will be honored verbatim.
It is up to the user to take caution in changing this flag; updating the
flag without also manually moving any existing metadata will cause Kudu
to fail at startup.
Aside from the update to the directory location, codepaths that
previously assumed that the first data directory _must_ be healthy have
been updated. The remaining invariant is that at least a single data
directory must be healthy.
A new test cases is added to fs_manager-test, and a test case in
data_dirs-test is updated as a sanity check to show that we can now open
the directory manager with a failed first data directory (previously
this codepath with lead to D/CHECK failures).
Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
---
M src/kudu/fs/block_manager_util.cc
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
6 files changed, 142 insertions(+), 40 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/1
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 )
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
Patch Set 12: Code-Review+2
Since this is a pretty significant change, maybe get another reviewer too?
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 12
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
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 18 Jan 2018 03:36:24 +0000
Gerrit-HasComments: No
[kudu-CR] fs: move metadata to the WAL directory
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9027
to look at the new patch set (#5).
Change subject: fs: move metadata to the WAL directory
......................................................................
fs: move metadata to the WAL directory
Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
---
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
2 files changed, 8 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/5
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 5
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: Tidy Bot
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 )
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
Patch Set 6:
(7 comments)
first pass. mostly looked at the test.
it'd be nice to have a more integration-y test that tests at least the most worrysome cases (downgrade/upgrade most likely scenarios)
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/block_manager_util.cc
File src/kudu/fs/block_manager_util.cc:
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/block_manager_util.cc@176
PS6, Line 176: LoadInstnaces
typo
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@248
PS6, Line 248: "wal"
could we be more specific? maybe look for full suffix?
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@255
PS6, Line 255: LOG(INFO) << s.ToString();
leftover from testing? if you want to print the status on failure you can do so on the assert below
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@258
PS6, Line 258: env_->DeleteRecursively(GetTestPath("wal"));
: env_->DeleteRecursively(GetTestPath("data"));
check the status
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@263
PS6, Line 263: opts.metadata_root = GetTestPath("data");
: ReinitFsManagerWithOpts(opts);
: ASSERT_OK(fs_manager()->CreateInitialFileSystemLayout());
: ASSERT_OK(fs_manager()->Open());
: ASSERT_STR_CONTAINS(fs_manager()->GetTabletMetadataDir(), "data");
in the case before this one you test with a metadata root path that does not exist, before you delete the data/wal could you test that if the metadata root is set to the old value (like in this case) it fails too?
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@269
PS6, Line 269: // Opening the FsManager with an empty fs_metadata_dir flag should account
: // for the old default and use the first data directory for metadata.
wait, empty string is different from "unset"? why?
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@276
PS6, Line 276: env_->DeleteRecursively(GetTestPath("wal"));
: env_->DeleteRecursively(GetTestPath("data"));
same
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 6
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
Gerrit-Comment-Date: Tue, 16 Jan 2018 20:07:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 )
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
Patch Set 6:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG@20
PS4, Line 20: It is up to the user to take caution in changing this flag; updating the
: flag without also manually moving any existing metadata will cause Kudu
: to fail at startup.
Isn't the same true of fs_wal_dir and fs_data_dirs? Or is changing fs_metadata_dir somehow more dangerous?
I guess what I'm asking is: why call this out explicitly if it works the same way as those other flags?
http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG@29
PS4, Line 29: cases
Nit: case
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/block_manager_util.cc
File src/kudu/fs/block_manager_util.cc:
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/block_manager_util.cc@176
PS4, Line 176: DCHECK_NE(first_healthy, -1); // Guaranteed by DataDirManager::LoadInstnaces().
Can we not depend on this, and instead return a bad status if there are no healthy instances? Seems like a weird dependency since this is a "util" function that ostensibly could be called from anywhere.
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@258
PS4, Line 258: env_->DeleteRecursively(GetTestPath("wal"));
: env_->DeleteRecursively(GetTestPath("data"));
Maybe use different tests so you needn't clean up in between?
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@271
PS4, Line 271: opts.metadata_root = "";
Maybe opts.metadata_root.clear() would be more idiomatic?
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@284
PS4, Line 284: ASSERT_OK(fs_manager()->Open());
After this could you check that the WAL root and the metadata root aren't the same?
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@288
PS4, Line 288: opts.metadata_root = "";
.clear() here too.
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@517
PS4, Line 517: // Try to open with a new data dir at the front of the list.
Nit: the other "Try to ..." comments here are all followed up by "this should fail", but this no longer fails. Reword?
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@524
PS4, Line 524: // But adding a new data dir elsewhere in the list is OK.
And reword this too.
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.h@90
PS4, Line 90: or the first configured data root if it already contains
: // existing metadata.
Would it be clearer if we additionally specified that this fallback behavior only takes effect when opening an existing filesystem? That is, when creating a new filesystem, it's just "verbatim, or WAL dir if empty"?
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@93
PS4, Line 93: DEFINE_string(fs_metadata_dir, "",
There should be a mention here of the fallback behavior for existing systems.
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@94
PS4, Line 94: Must be equivalent to fs_wal_dir or a
I thought this wasn't true anymore?
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@95
PS4, Line 95: fs_dataA_dirs
fs_data_dirs
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@253
PS4, Line 253: // Check the first data root for metadata.
Could you LOG here what we actually used for the metadata directory, a la L243-244?
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@256
PS4, Line 256: // If there is no metadata in the first data root, use the WAL root.
Hmm, but we wouldn't want to do this fallback when creating a brand new filesystem. Does it actually matter? Is it possible for CreateFileSystemLayout to succeed if meta_dir_in_data_root already exists?
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@265
PS4, Line 265: const auto& canonicalized_metadata_root = canonicalized_metadata_fs_root_;
Why do we need this local?
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@421
PS4, Line 421: if (dd_manager_->FindUuidIndexByRoot(canonicalized_metadata_fs_root_.path, &metadata_idx) &&
I'm confused; why would the DataDirManager be aware of the metadata root, since it's no longer guaranteed to be colocated with the data directory?
Oh, this is just for the case where they are colocated? What's the value in this check, then, since it won't fire for new deployments?
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@547
PS4, Line 547: .
Nit: drop the period
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 6
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: Tidy Bot
Gerrit-Comment-Date: Tue, 16 Jan 2018 19:09:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9027
to look at the new patch set (#8).
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
KUDU-1489: allow configuration of metadata dir
Metadata files currently reside in the first configured data directory,
which isn't always the best choice; this first drive, if not performant,
can act as a bottleneck. Often times the drive containing the WALs is a
better choice, as it is recommended to be the fastest.
This patch allows users to configure their metadata directory through
the new fs_metadata_dir flag. If empty, Kudu will check if a metadata
directory exists in the first member of fs_data_dirs, and if none
exists, Kudu will use fs_wal_dir as the root directory for metadata.
Otherwise, the flag will be honored verbatim.
Aside from the update to the directory location, codepaths that
previously assumed that the first data directory _must_ be healthy have
been updated. The remaining invariant is that at least a single data
directory must be healthy.
The following tests are added:
- fs_manager-test: unit tests for various scenarios
- data_dirs-test: a test case is updated to show that we can now open
the directory manager with a failed first data directory (previously
this codepath would hit D/CHECK failures)
- mini_tablet_server-test: an end-to-end test is added to switch back
and forth between the old and new default metadata directories
- kudu-tool-test: a small test that tools still work, but only when the
appropriate FS flags are provided
Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
---
M src/kudu/fs/block_manager_util.cc
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tserver/mini_tablet_server-test.cc
10 files changed, 316 insertions(+), 69 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/8
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 8
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] KUDU-1489: allow configuration of metadata dir
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 )
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
Patch Set 11:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/fs/fs_manager-test.cc@265
PS11, Line 265: // We should be able to explicate that the metadata is in the WAL root.
Nit: is 'explicate' the right choice here? I substitute it with 'analyze' or 'explain' and the sentence doesn't make sense.
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/fs/fs_manager.h@268
PS11, Line 268: friend class tserver::MiniTabletServerTest_TestFsLayoutEndToEnd_Test;
Isn't this exactly the same as FRIEND_TEST(MiniTabletServerTest...) above?
Actually, maybe the above has no effect because you should be doing FRIEND_TEST(tserver::MiniTabletServerTest, ...)?
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/kudu-tool-test.cc@2214
PS11, Line 2214: env_->CreateDir(kTestDir);
ASSERT_OK
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/kudu-tool-test.cc@2229
PS11, Line 2229: RunTool(Substitute("fs dump uuid --fs_wal_dir=$0", opts.wal_root), nullptr, &stderr, {}, {});
ASSERT on the expected status?
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/tool_action_fs.cc
File src/kudu/tools/tool_action_fs.cc:
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/tool_action_fs.cc@764
PS11, Line 764: .AddOptionalParameter("fs_metadata_dir")
Since you're adding these, mind reordering the optional parameters to be alphabetically sorted? No backwards compatibility concerns; it'll just show them differently in the help.
Same for the other files.
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tserver/mini_tablet_server-test.cc
File src/kudu/tserver/mini_tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tserver/mini_tablet_server-test.cc@60
PS11, Line 60: younger
Nit: older
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 11
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
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 17 Jan 2018 22:24:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 )
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
Patch Set 4:
I think we should merge KUDU-2202 before merging this (will need to update a couple of things), but feel free to review this in it's current form.
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 4
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: Tidy Bot
Gerrit-Comment-Date: Tue, 16 Jan 2018 18:48:34 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 )
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
Patch Set 12:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/fs/fs_manager-test.cc@265
PS11, Line 265: // We should be able to verify that the metadata is in the WAL root.
> Nit: is 'explicate' the right choice here? I substitute it with 'analyze' o
You're right. Wanted to say that we should be able to _explicitly see_. I'll go with "verify".
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/fs/fs_manager.h@268
PS11, Line 268: // Initializes, sanitizes, and canonicalizes the filesystem roots.
> Isn't this exactly the same as FRIEND_TEST(MiniTabletServerTest...) above?
Done
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/kudu-tool-test.cc@2214
PS11, Line 2214: ASSERT_OK(env_->CreateDir(kTestDir));
> ASSERT_OK
Done
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/kudu-tool-test.cc@2229
PS11, Line 2229: Status s = RunTool(Substitute("fs dump uuid --fs_wal_dir=$0", opts.wal_root),
> ASSERT on the expected status?
Ah, yeah. Thought it was void like RunAction*.
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/tool_action_fs.cc
File src/kudu/tools/tool_action_fs.cc:
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/tool_action_fs.cc@764
PS11, Line 764: .AddOptionalParameter("fs_wal_dir")
> Since you're adding these, mind reordering the optional parameters to be al
Done
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tserver/mini_tablet_server-test.cc
File src/kudu/tserver/mini_tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tserver/mini_tablet_server-test.cc@60
PS11, Line 60: ot), or
> Nit: older
Done
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 12
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
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 18 Jan 2018 02:24:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9027
to look at the new patch set (#12).
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
KUDU-1489: allow configuration of metadata dir
Metadata files currently reside in the first configured data directory,
which isn't always the best choice; this first drive, if not performant,
can act as a performance bottleneck. Often times the drive containing
the WALs is a better choice, as it is recommended to be the fastest.
This patch allows users to configure their metadata directory through
the new fs_metadata_dir flag. If empty, Kudu will check if a metadata
directory exists in the first member of fs_data_dirs, and if none
exists, Kudu will use fs_wal_dir as the root directory for metadata.
Otherwise, the flag will be honored verbatim.
Aside from the update to the directory location, codepaths that
previously assumed that the first data directory _must_ be healthy have
been updated. The remaining invariant is that at least a single data
directory must be healthy.
The following test changes are included:
- fs_manager-test: unit tests for various scenarios
- data_dirs-test: a test case is updated to show that we can now open
the directory manager with a failed first data directory (previously
this codepath would hit D/CHECK failures)
- mini_tablet_server-test: an end-to-end test is added to manually
go back and forth between the old and new default metadata directories
- kudu-tool-test: a small test that tools still work, but only when the
appropriate FS flags are provided
Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
---
M src/kudu/fs/block_manager_util.cc
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/mini_tablet_server-test.cc
11 files changed, 347 insertions(+), 87 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/12
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 12
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
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9027
to look at the new patch set (#10).
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
KUDU-1489: allow configuration of metadata dir
Metadata files currently reside in the first configured data directory,
which isn't always the best choice; this first drive, if not performant,
can act as a performance bottleneck. Often times the drive containing
the WALs is a better choice, as it is recommended to be the fastest.
This patch allows users to configure their metadata directory through
the new fs_metadata_dir flag. If empty, Kudu will check if a metadata
directory exists in the first member of fs_data_dirs, and if none
exists, Kudu will use fs_wal_dir as the root directory for metadata.
Otherwise, the flag will be honored verbatim.
Aside from the update to the directory location, codepaths that
previously assumed that the first data directory _must_ be healthy have
been updated. The remaining invariant is that at least a single data
directory must be healthy.
The following test changes are included:
- fs_manager-test: unit tests for various scenarios
- data_dirs-test: a test case is updated to show that we can now open
the directory manager with a failed first data directory (previously
this codepath would hit D/CHECK failures)
- mini_tablet_server-test: an end-to-end test is added to manually
go back and forth between the old and new default metadata directories
- kudu-tool-test: a small test that tools still work, but only when the
appropriate FS flags are provided
Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
---
M src/kudu/fs/block_manager_util.cc
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/mini_tablet_server-test.cc
11 files changed, 327 insertions(+), 69 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/10
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 10
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] KUDU-1489: allow configuration of metadata dir
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9027
to look at the new patch set (#9).
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
KUDU-1489: allow configuration of metadata dir
Metadata files currently reside in the first configured data directory,
which isn't always the best choice; this first drive, if not performant,
can act as a performance bottleneck. Often times the drive containing
the WALs is a better choice, as it is recommended to be the fastest.
This patch allows users to configure their metadata directory through
the new fs_metadata_dir flag. If empty, Kudu will check if a metadata
directory exists in the first member of fs_data_dirs, and if none
exists, Kudu will use fs_wal_dir as the root directory for metadata.
Otherwise, the flag will be honored verbatim.
Aside from the update to the directory location, codepaths that
previously assumed that the first data directory _must_ be healthy have
been updated. The remaining invariant is that at least a single data
directory must be healthy.
The following test changes are included:
- fs_manager-test: unit tests for various scenarios
- data_dirs-test: a test case is updated to show that we can now open
the directory manager with a failed first data directory (previously
this codepath would hit D/CHECK failures)
- mini_tablet_server-test: an end-to-end test is added to manually
go back and forth between the old and new default metadata directories
- kudu-tool-test: a small test that tools still work, but only when the
appropriate FS flags are provided
Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
---
M src/kudu/fs/block_manager_util.cc
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/mini_tablet_server-test.cc
11 files changed, 327 insertions(+), 69 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/9
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 9
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] KUDU-1489: allow configuration of metadata dir
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 )
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
Patch Set 6:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG@20
PS4, Line 20: It is up to the user to take caution in changing this flag; updating the
: flag without also manually moving any existing metadata will cause Kudu
: to fail at startup.
> Isn't the same true of fs_wal_dir and fs_data_dirs? Or is changing fs_metad
Good point. No point in bringing it up, I'll take this out.
http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG@29
PS4, Line 29: cases
> Nit: case
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/block_manager_util.cc
File src/kudu/fs/block_manager_util.cc:
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/block_manager_util.cc@176
PS4, Line 176: DCHECK_NE(first_healthy, -1); // Guaranteed by DataDirManager::LoadInstnaces().
> Can we not depend on this, and instead return a bad status if there are no
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@258
PS4, Line 258: env_->DeleteRecursively(GetTestPath("wal"));
: env_->DeleteRecursively(GetTestPath("data"));
> Maybe use different tests so you needn't clean up in between?
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@271
PS4, Line 271: opts.metadata_root = "";
> Maybe opts.metadata_root.clear() would be more idiomatic?
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@284
PS4, Line 284: ASSERT_OK(fs_manager()->Open());
> After this could you check that the WAL root and the metadata root aren't t
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@288
PS4, Line 288: opts.metadata_root = "";
> .clear() here too.
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@517
PS4, Line 517: // Try to open with a new data dir at the front of the list.
> Nit: the other "Try to ..." comments here are all followed up by "this shou
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@524
PS4, Line 524: // But adding a new data dir elsewhere in the list is OK.
> And reword this too.
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.h@90
PS4, Line 90: or the first configured data root if it already contains
: // existing metadata.
> Would it be clearer if we additionally specified that this fallback behavio
Hm, I didn't explicitly call it out, but I reworded it to hopefully clarify that the first data dir is only used if metadata exists there from a previous deployment. Let me know what you think.
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@93
PS4, Line 93: DEFINE_string(fs_metadata_dir, "",
> There should be a mention here of the fallback behavior for existing system
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@94
PS4, Line 94: Must be equivalent to fs_wal_dir or a
> I thought this wasn't true anymore?
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@95
PS4, Line 95: fs_dataA_dirs
> fs_data_dirs
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@253
PS4, Line 253: // Check the first data root for metadata.
> Could you LOG here what we actually used for the metadata directory, a la L
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@256
PS4, Line 256: // If there is no metadata in the first data root, use the WAL root.
> Hmm, but we wouldn't want to do this fallback when creating a brand new fil
Is your concern that if the FS layout already partially exists, but not completely (such that fs_manager()->Open() fails, but fs_manager()->Create() succeeds), we may end up going through CreateInitialFileSystemLayout() with the first data root storing metadata?
That could happen if they fail to start up a cluster with Kudu 1.6, upgrade to Kudu 1.7, and then start up successfully. I think I'm ok with this though, although a simple update would be to also check whether the metadata directory is empty, in which case we could use the WAL dir. What do you think?
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@265
PS4, Line 265: const auto& canonicalized_metadata_root = canonicalized_metadata_fs_root_;
> Why do we need this local?
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@421
PS4, Line 421: if (dd_manager_->FindUuidIndexByRoot(canonicalized_metadata_fs_root_.path, &metadata_idx) &&
> I'm confused; why would the DataDirManager be aware of the metadata root, s
Hm, good point. It's not important and it's just noise moving forward.
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@547
PS4, Line 547: .
> Nit: drop the period
Debugging string, dropping it altogether.
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 6
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
Gerrit-Comment-Date: Tue, 16 Jan 2018 20:25:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 )
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
Patch Set 11:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/data_dirs.cc@728
PS10, Line 728: CHECK_NE(first_healthy, -1); // Guaranteed by LoadInstances().
> maybe just make this a CHECK since it'll avoid some ugly stack overwriting
Done
http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc@94
PS10, Line 94: compatibility
> typo
Done
http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc@96
PS10, Line 96: exist
> nit: exists
Done
http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc@98
PS10, Line 98: TAG_FLAG(fs_metadata_dir, stable);
> maybe this should be stable out of the gate?
Done
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 11
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
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 17 Jan 2018 07:29:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9027
to look at the new patch set (#6).
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
KUDU-1489: allow configuration of metadata dir
Metadata files currently reside in the first configured data directory,
which isn't always the best choice; this first drive, if not performant,
can act as a bottleneck. Often times the drive containing the WALs is a
better choice, as it is recommended to be the fastest.
This patch allows users to configure their metadata directory through
the new fs_metadata_dir flag. If empty, Kudu will check if a metadata
directory exists in the first member of fs_data_dirs, and if none
exists, Kudu will use fs_wal_dir as the root directory for metadata.
Otherwise, the flag will be honored verbatim.
It is up to the user to take caution in changing this flag; updating the
flag without also manually moving any existing metadata will cause Kudu
to fail at startup.
Aside from the update to the directory location, codepaths that
previously assumed that the first data directory _must_ be healthy have
been updated. The remaining invariant is that at least a single data
directory must be healthy.
A new test cases is added to fs_manager-test, and a test case in
data_dirs-test is updated as a sanity check to show that we can now open
the directory manager with a failed first data directory (previously
this codepath would hit D/CHECK failures).
Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
---
M src/kudu/fs/block_manager_util.cc
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
7 files changed, 160 insertions(+), 58 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/6
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 6
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: Tidy Bot
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 )
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
Patch Set 12: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 12
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
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 18 Jan 2018 20:08:06 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 )
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9027/7/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/9027/7/src/kudu/fs/fs_manager-test.cc@534
PS7, Line 534: // Adding a new data dir elsewhere in the list is OK.
This isn't what the test does anymore though; it's just shuffling the order of the directories in data_roots. Is that worth testing? If so, reword the comment further. If not, remove this part of the test.
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@256
PS4, Line 256: const string meta_dir_in_data_root = JoinPathSegments(canonicalized_data_fs_roots_[0].path,
> Is your concern that if the FS layout already partially exists, but not com
My concern is with the following sequence:
1. I'm going to create an FS with wal_dir=/a, metadata_dir="", data_dirs=/b.
2. First I do "mkdir -p /b/tablet-meta".
3. Then I do FSManager.CreateInitialFileSystemLayout.
4. Then I do FSManager.Open.
I think I'll wind up with metadata_dir=/b/tablet-meta instead of /a/tablet-meta, but maybe this is too contrived due to step #2. What would be interesting is if step #2 could be substituted with an otherwise valid Kudu operation.
Anyway, my question was "will step #3 fail because of the directory created in step #2"? If so, this is a non-issue.
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 7
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
Gerrit-Comment-Date: Tue, 16 Jan 2018 21:23:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9027
to look at the new patch set (#3).
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
KUDU-1489: allow configuration of metadata dir
Metadata files currently reside in the first configured data directory,
which isn't always the best choice; this first drive, if not performant,
can act as a bottleneck. Often times the drive containing the WALs is a
better choice, as it is recommended to be the fastest.
This patch allows users to configure their metadata directory through
the new fs_metadata_dir flag. If empty, Kudu will check if a metadata
directory exists in the first member of fs_data_dirs, and if none
exists, Kudu will use fs_wal_dir as the root directory for metadata.
Otherwise, the flag will be honored verbatim.
It is up to the user to take caution in changing this flag; updating the
flag without also manually moving any existing metadata will cause Kudu
to fail at startup.
Aside from the update to the directory location, codepaths that
previously assumed that the first data directory _must_ be healthy have
been updated. The remaining invariant is that at least a single data
directory must be healthy.
A new test cases is added to fs_manager-test, and a test case in
data_dirs-test is updated as a sanity check to show that we can now open
the directory manager with a failed first data directory (previously
this codepath would hit D/CHECK failures).
Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
---
M src/kudu/fs/block_manager_util.cc
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
6 files changed, 143 insertions(+), 40 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/3
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 )
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
Patch Set 10:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/data_dirs.cc@728
PS10, Line 728: DCHECK_NE(first_healthy, -1); // Guaranteed by LoadInstances().
maybe just make this a CHECK since it'll avoid some ugly stack overwriting on the next line, and this isn't any sort of perf-critical code
http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc@94
PS10, Line 94: compatability
typo
http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc@96
PS10, Line 96: exist
nit: exists
http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc@98
PS10, Line 98: TAG_FLAG(fs_metadata_dir, advanced);
maybe this should be stable out of the gate?
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 10
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
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 17 Jan 2018 06:07:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9027 )
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
KUDU-1489: allow configuration of metadata dir
Metadata files currently reside in the first configured data directory,
which isn't always the best choice; this first drive, if not performant,
can act as a performance bottleneck. Often times the drive containing
the WALs is a better choice, as it is recommended to be the fastest.
This patch allows users to configure their metadata directory through
the new fs_metadata_dir flag. If empty, Kudu will check if a metadata
directory exists in the first member of fs_data_dirs, and if none
exists, Kudu will use fs_wal_dir as the root directory for metadata.
Otherwise, the flag will be honored verbatim.
Aside from the update to the directory location, codepaths that
previously assumed that the first data directory _must_ be healthy have
been updated. The remaining invariant is that at least a single data
directory must be healthy.
The following test changes are included:
- fs_manager-test: unit tests for various scenarios
- data_dirs-test: a test case is updated to show that we can now open
the directory manager with a failed first data directory (previously
this codepath would hit D/CHECK failures)
- mini_tablet_server-test: an end-to-end test is added to manually
go back and forth between the old and new default metadata directories
- kudu-tool-test: a small test that tools still work, but only when the
appropriate FS flags are provided
Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Reviewed-on: http://gerrit.cloudera.org:8080/9027
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/fs/block_manager_util.cc
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/mini_tablet_server-test.cc
11 files changed, 347 insertions(+), 87 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, approved
Todd Lipcon: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 13
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
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 )
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
Patch Set 7:
(9 comments)
Done, and added a more integration-y test per David's feedback.
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/block_manager_util.cc
File src/kudu/fs/block_manager_util.cc:
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/block_manager_util.cc@176
PS6, Line 176:
> typo
Done
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@248
PS6, Line 248: "wal"
> could we be more specific? maybe look for full suffix?
Done
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@255
PS6, Line 255: LOG(INFO) << s.ToString();
> leftover from testing? if you want to print the status on failure you can d
Done
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@258
PS6, Line 258:
: TEST_F(FsManagerTestBase, TestMetadataDirInData
> check the status
Per Adar's comment, I moved these each into their own test.
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@263
PS6, Line 263:
: // Creating a brand new FS layout configured with metadata in the first data
: // directory emulates the default behavior in Kudu 1.6 and below.
: opts.metadata_root = GetTestPath("data");
: ReinitFsManagerWithOpts(opts);
> in the case before this one you test with a metadata root path that does no
Done
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@269
PS6, Line 269: ASSERT_OK(fs_manager()->Open());
: ASSERT_STR_CONTAINS(fs_manager()->GetTabletMetadataDir(), "data");
> wait, empty string is different from "unset"? why?
Mm, not sure I understand, what do you mean by "unset" here?
This behavior is for backwards compatibility: older deployments can continue running with no change to their configurations (i.e. with an empty fs_metadata_dir flag) while maintaining that their metadata will be in the first data dir. New deployments will use the fs_wal_dir for metadata if fs_metadata_dir is empty.
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@276
PS6, Line 276: ASSERT_OK(fs_manager()->Open());
: ASSERT_STR_CONTAINS(fs_manager()->GetTabletMe
> same
Ack
http://gerrit.cloudera.org:8080/#/c/9027/7/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/9027/7/src/kudu/fs/fs_manager-test.cc@534
PS7, Line 534: // Adding a new data dir elsewhere in the list is OK.
> This isn't what the test does anymore though; it's just shuffling the order
Ah you're right, my bad. Removing it. I'll add a similar test up where we test metadata in the first data root.
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@256
PS4, Line 256: const string meta_dir_in_data_root = JoinPathSegments(canonicalized_data_fs_roots_[0].path,
> My concern is with the following sequence:
I see. Step 3 would fail at CreateFileSystemRoots(). The canonicalized roots wouldn't be empty, so this would lead to an AlreadyPresent error.
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 7
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
Gerrit-Comment-Date: Wed, 17 Jan 2018 02:57:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 )
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
Patch Set 5:
Sorry, pushed accidentally in conjunction with another patch. Reverting to patch set 4.
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 5
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: Tidy Bot
Gerrit-Comment-Date: Tue, 16 Jan 2018 18:51:48 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9027
to look at the new patch set (#7).
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
KUDU-1489: allow configuration of metadata dir
Metadata files currently reside in the first configured data directory,
which isn't always the best choice; this first drive, if not performant,
can act as a bottleneck. Often times the drive containing the WALs is a
better choice, as it is recommended to be the fastest.
This patch allows users to configure their metadata directory through
the new fs_metadata_dir flag. If empty, Kudu will check if a metadata
directory exists in the first member of fs_data_dirs, and if none
exists, Kudu will use fs_wal_dir as the root directory for metadata.
Otherwise, the flag will be honored verbatim.
Aside from the update to the directory location, codepaths that
previously assumed that the first data directory _must_ be healthy have
been updated. The remaining invariant is that at least a single data
directory must be healthy.
New test cases are added to fs_manager-test, and a test case in
data_dirs-test is updated as a sanity check to show that we can now open
the directory manager with a failed first data directory (previously
this codepath would hit D/CHECK failures).
Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
---
M src/kudu/fs/block_manager_util.cc
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
7 files changed, 173 insertions(+), 64 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/7
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 7
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] KUDU-1489: allow configuration of metadata dir
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9027
to look at the new patch set (#4).
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
KUDU-1489: allow configuration of metadata dir
Metadata files currently reside in the first configured data directory,
which isn't always the best choice; this first drive, if not performant,
can act as a bottleneck. Often times the drive containing the WALs is a
better choice, as it is recommended to be the fastest.
This patch allows users to configure their metadata directory through
the new fs_metadata_dir flag. If empty, Kudu will check if a metadata
directory exists in the first member of fs_data_dirs, and if none
exists, Kudu will use fs_wal_dir as the root directory for metadata.
Otherwise, the flag will be honored verbatim.
It is up to the user to take caution in changing this flag; updating the
flag without also manually moving any existing metadata will cause Kudu
to fail at startup.
Aside from the update to the directory location, codepaths that
previously assumed that the first data directory _must_ be healthy have
been updated. The remaining invariant is that at least a single data
directory must be healthy.
A new test cases is added to fs_manager-test, and a test case in
data_dirs-test is updated as a sanity check to show that we can now open
the directory manager with a failed first data directory (previously
this codepath would hit D/CHECK failures).
Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
---
M src/kudu/fs/block_manager_util.cc
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
7 files changed, 160 insertions(+), 58 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/4
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9027
to look at the new patch set (#11).
Change subject: KUDU-1489: allow configuration of metadata dir
......................................................................
KUDU-1489: allow configuration of metadata dir
Metadata files currently reside in the first configured data directory,
which isn't always the best choice; this first drive, if not performant,
can act as a performance bottleneck. Often times the drive containing
the WALs is a better choice, as it is recommended to be the fastest.
This patch allows users to configure their metadata directory through
the new fs_metadata_dir flag. If empty, Kudu will check if a metadata
directory exists in the first member of fs_data_dirs, and if none
exists, Kudu will use fs_wal_dir as the root directory for metadata.
Otherwise, the flag will be honored verbatim.
Aside from the update to the directory location, codepaths that
previously assumed that the first data directory _must_ be healthy have
been updated. The remaining invariant is that at least a single data
directory must be healthy.
The following test changes are included:
- fs_manager-test: unit tests for various scenarios
- data_dirs-test: a test case is updated to show that we can now open
the directory manager with a failed first data directory (previously
this codepath would hit D/CHECK failures)
- mini_tablet_server-test: an end-to-end test is added to manually
go back and forth between the old and new default metadata directories
- kudu-tool-test: a small test that tools still work, but only when the
appropriate FS flags are provided
Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
---
M src/kudu/fs/block_manager_util.cc
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/mini_tablet_server-test.cc
11 files changed, 328 insertions(+), 69 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/11
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 11
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
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>