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/08/07 17:46:50 UTC
[kudu-CR] separate DataDirManager from BlockManagers
Andrew Wong has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7602
Change subject: separate DataDirManager from BlockManagers
......................................................................
separate DataDirManager from BlockManagers
Currently, the DataDirManager is owned by the BlockManagers. Since only
blocks are placed in 'data dirs' (i.e. subdirectories named 'data' under
the roots specified by 'fs_data_dirs'), this hierarchy made sense.
However, it would be nice to track other files that fall within
'fs_data_dirs' (e.g. tablet-metadata, consensus-metadata). Splitting the
directory manager from the block manager makes this more feasible.
Some logic previously in the FsManager and BlockManagers
is moved into the DataDirManager:
- canonicalization of data roots now occurs in DataDirManager, ensuring
that the DataDirManager will know about all data directories, even
those that fail to open/canonicalize
- BlockManagers no longer have a Create() function
- BlockManager's dtor will now just wait for DataDir closures to finish
instead of shutting down the DataDirManager as to not directory affect
the DataDirManager
Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
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/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
14 files changed, 420 insertions(+), 278 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/7602/1
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] separate DataDirManager from BlockManagers
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7602
to look at the new patch set (#13).
Change subject: separate DataDirManager from BlockManagers
......................................................................
separate DataDirManager from BlockManagers
Currently, the DataDirManager is owned by the BlockManagers. Since only
blocks are placed in 'data dirs' (i.e. subdirectories named 'data' under
the roots specified by 'fs_data_dirs'), this hierarchy made sense.
However, it would be nice to track other files that fall within
'fs_data_dirs' (e.g. tablet-metadata, consensus-metadata). Splitting the
directory manager from the block manager makes this more feasible.
This patch introduces a number of changes to the FsManager,
BlockManager, and DataDirManager with respect to initialization.
- BlockManagers no longer have a Create() function
- BlockManager's dtor will now just wait for DataDir closures to finish
instead of shutting down the DataDirManager as to not directly affect
the DataDirManager
- DataDirManagers now have two static constructors: one to open an
existing layout, and another to create and open a new layout
- FsManagers will only create the BlockManager when opening an fs layout
- FsManagers will only Open() a DataDirManager if one has not already
been constructed
- A validator is added to check the value of FLAGS_block_manager
before any of the above initialization can occur
Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
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/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
16 files changed, 443 insertions(+), 306 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/7602/13
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
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: Tidy Bot
[kudu-CR] separate DataDirManager from BlockManagers
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/7602
to look at the new patch set (#4).
Change subject: separate DataDirManager from BlockManagers
......................................................................
separate DataDirManager from BlockManagers
Currently, the DataDirManager is owned by the BlockManagers. Since only
blocks are placed in 'data dirs' (i.e. subdirectories named 'data' under
the roots specified by 'fs_data_dirs'), this hierarchy made sense.
However, it would be nice to track other files that fall within
'fs_data_dirs' (e.g. tablet-metadata, consensus-metadata). Splitting the
directory manager from the block manager makes this more feasible.
Some logic previously in the FsManager and BlockManagers is moved into
the DataDirManager:
- canonicalization of data roots now occurs in DataDirManager, ensuring
that the DataDirManager will know about all data directories, even
those that fail to open/canonicalize
- BlockManagers no longer have a Create() function
- BlockManager's dtor will now just wait for DataDir closures to finish
instead of shutting down the DataDirManager as to not directory affect
the DataDirManager
To clarify some vocabulary:
- Root: a top-level directory specified by 'fs_data_dirs'
- Data (root) dir: a subdirectory of a data root, named 'data'. Blocks
are placed in this directory.
Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
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/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
16 files changed, 525 insertions(+), 330 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/7602/4
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] separate DataDirManager from BlockManagers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: separate DataDirManager from BlockManagers
......................................................................
Patch Set 7:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/7602/7/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:
Line 146: bm_.reset();
Why does the block manager have to be destroyed separately up here? Why isn't the reset on L154 sufficient?
Oh, in log_block_manager-test you have a comment explaining this. Can you add it here too?
http://gerrit.cloudera.org:8080/#/c/7602/7/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
Line 289: LOG(INFO) << "Constructing a new DataDirManager";
Still need this?
Line 292: metrics_.reset(new DataDirMetrics(std::move(opts.metric_entity)));
> warning: passing result of std::move() as a const reference argument; no mo
Ah, I see, this is because DataDirMetrics takes the MetricEntity by cref. And it doesn't really make sense to change that, since DataDirMetrics doesn't take a ref itself. But, storing the MetricEntity in 'opts' by value makes sense, so just dropping the std::move() here should be sufficient.
PS7, Line 339: canonicalized_data_roots.insert(canonicalized_data_roots.end(),
: canonicalized_data_roots_set.begin(),
: canonicalized_data_roots_set.end());
Isn't this logically the same as the returned values from the calls to FsManager::CanonicalizePath? What's the purpose of all of the logic in between?
PS7, Line 350: const int kFlagCreateTestHolePunch = 0x1;
: const int kFlagCreateFsync = 0x2;
But why bother with flags at all? Flags are a means of defining an API and there's no API here. That is, where you have "flags & kFlagCreateFsync" you could just have "true". And where you have "flags & kFlagCreateTestHolePunch" you could just have "block_manager_type_ == "log"".
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
PS6, Line 229: // Shuts down all directories' thread pools.
: void Shutdown();
:
: // Waits on all directories' thread pools.
: void WaitOnClosures();
:
: // Initializes the data directories on disk.
: //
: // Returns an error if initialized directories already exist.
: Status Create();
:
: // Opens existing data roots from disk and indexes the files found.
: //
> I whittled it down to Init(), Create(), and Open(). The logic in Init() (ca
Would it be possible to convert the static Init and the Create and/or Open into just "static CreateNew" and "static LoadExisting"? That would be simpler, IMHO. Couldn't the canonicalization be called privately by CreateNew/LoadExisting? I get that both FsManager and BlockManager hew to this "call Create() AND Open() when creating a new thing, call just Open() to open an existing thing" pattern, but it's pretty confusing and inconsistent with other objects in the Kudu codebase.
Also, Init() isn't a great name for what is now a factory method, since elsewhere we use Init() for "initialize more state in this existing object". Better to use Create() for that.
http://gerrit.cloudera.org:8080/#/c/7602/7/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
PS7, Line 200: Defaultas to fales
Defaults to false
Also nit: having this be on a new line (like L197) would be more consistent with how you've done metric_entity.
Line 378: // The canonicalized roots provided at construction time, taken verbatim.
I understand what this means, but "construction time" is a little weird because it doesn't refer to the time that the user constructed the object (i.e. at Init() time, when the roots were _not yet_ canonicalized), but to the time that the private constructor was called by Init(). Might be less confusing to reword.
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
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: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] separate DataDirManager from BlockManagers
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: separate DataDirManager from BlockManagers
......................................................................
Patch Set 8:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7602/8/src/kudu/fs/block_manager-stress-test.cc
File src/kudu/fs/block_manager-stress-test.cc:
PS8, Line 144:
:
:
:
:
:
:
:
> Don't we need to preserve this in some capacity?
Done
http://gerrit.cloudera.org:8080/#/c/7602/8/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
PS8, Line 231: qcquired
> acquired
Done
http://gerrit.cloudera.org:8080/#/c/7602/8/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
Line 164
> Maybe restore this comment?
Done
Line 71: return value == "log" || value == "file";
> On macOS can you set this up so that "file" is the only valid option?
Done
Line 270: RETURN_NOT_OK(DataDirManager::OpenExisting(env_,
> Hmm, but in the case of FsManager::CreateNewFileSystemLayout followed by Fs
Yeah, it actually seems that this is mandatory. I was banging my head trying to figure out the test failures, and it seems like the double-initting was causing an early release of file locks.
Anyway, the simple fix is to not run this block if dd_manager_ has already been constructed. A slightly more complicated fix might require changing the create/open of FsManager in a similar way, but I think that would belong in a separate patch.
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] separate DataDirManager from BlockManagers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: separate DataDirManager from BlockManagers
......................................................................
separate DataDirManager from BlockManagers
Currently, the DataDirManager is owned by the BlockManagers. Since only
blocks are placed in 'data dirs' (i.e. subdirectories named 'data' under
the roots specified by 'fs_data_dirs'), this hierarchy made sense.
However, it would be nice to track other files that fall within
'fs_data_dirs' (e.g. tablet-metadata, consensus-metadata). Splitting the
directory manager from the block manager makes this more feasible.
This patch introduces a number of changes to the FsManager,
BlockManager, and DataDirManager with respect to initialization.
- BlockManagers no longer have a Create() function
- BlockManager's dtor will now just wait for DataDir closures to finish
instead of shutting down the DataDirManager as to not directly affect
the DataDirManager
- DataDirManagers now have two static constructors: one to open an
existing layout, and another to create and open a new layout
- FsManagers will only create the BlockManager when opening an fs layout
- FsManagers will only Open() a DataDirManager if one has not already
been constructed
- A validator is added to check the value of FLAGS_block_manager
before any of the above initialization can occur
Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Reviewed-on: http://gerrit.cloudera.org:8080/7602
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
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/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
16 files changed, 443 insertions(+), 306 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
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: Tidy Bot
[kudu-CR] separate DataDirManager from BlockManagers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: separate DataDirManager from BlockManagers
......................................................................
Patch Set 9: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7602/9/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
Line 381: metadata_.swap(metadata);
Why is this (and the other changes to 'metadata' in this function) necessary? Why can't we just populate metadata_ in Open() as before?
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
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: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] separate DataDirManager from BlockManagers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: separate DataDirManager from BlockManagers
......................................................................
Patch Set 8:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/7602/8/src/kudu/fs/block_manager-stress-test.cc
File src/kudu/fs/block_manager-stress-test.cc:
PS8, Line 144:
:
:
:
:
:
:
:
Don't we need to preserve this in some capacity?
http://gerrit.cloudera.org:8080/#/c/7602/7/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
Line 378: // Goes through the data dirs in 'uuid_indices' and populates
> Done. Ah interesting point; how about, "provided to the constructor"
Yeah, that's more clear.
http://gerrit.cloudera.org:8080/#/c/7602/8/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
PS8, Line 231: qcquired
acquired
http://gerrit.cloudera.org:8080/#/c/7602/8/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
Line 164
Maybe restore this comment?
Line 71: return value == "log" || value == "file";
On macOS can you set this up so that "file" is the only valid option?
Line 270: RETURN_NOT_OK(DataDirManager::OpenExisting(env_,
Hmm, but in the case of FsManager::CreateNewFileSystemLayout followed by FsManager::Open, we'll end up creating the DataDirManager twice, once in CreateNew() and once in OpenExisting().
It's not the biggest deal I guess, but maybe it's fixable without too much work?
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] separate DataDirManager from BlockManagers
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: separate DataDirManager from BlockManagers
......................................................................
Patch Set 13:
This patch hasn't changed much; mainly been rebasing it. It's been +2'd though without significant changes since.
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
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: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] separate DataDirManager from BlockManagers
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7602
to look at the new patch set (#10).
Change subject: separate DataDirManager from BlockManagers
......................................................................
separate DataDirManager from BlockManagers
Currently, the DataDirManager is owned by the BlockManagers. Since only
blocks are placed in 'data dirs' (i.e. subdirectories named 'data' under
the roots specified by 'fs_data_dirs'), this hierarchy made sense.
However, it would be nice to track other files that fall within
'fs_data_dirs' (e.g. tablet-metadata, consensus-metadata). Splitting the
directory manager from the block manager makes this more feasible.
This patch introduces a number of changes to the FsManager,
BlockManager, and DataDirManager with respect to initialization.
- BlockManagers no longer have a Create() function
- BlockManager's dtor will now just wait for DataDir closures to finish
instead of shutting down the DataDirManager as to not directly affect
the DataDirManager
- DataDirManagers now have two static constructors: one to open an
existing layout, and another to create and open a new layout
- FsManagers will only create the BlockManager when opening an fs layout
- FsManagers will only Open() a DataDirManager if one has not already
been constructed
- A validator is added to check the value of FLAGS_block_manager
before any of the above initialization can occur
Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
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/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
16 files changed, 443 insertions(+), 306 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/7602/10
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] separate DataDirManager from BlockManagers
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/7602
to look at the new patch set (#8).
Change subject: separate DataDirManager from BlockManagers
......................................................................
separate DataDirManager from BlockManagers
Currently, the DataDirManager is owned by the BlockManagers. Since only
blocks are placed in 'data dirs' (i.e. subdirectories named 'data' under
the roots specified by 'fs_data_dirs'), this hierarchy made sense.
However, it would be nice to track other files that fall within
'fs_data_dirs' (e.g. tablet-metadata, consensus-metadata). Splitting the
directory manager from the block manager makes this more feasible.
This introduces a number of changes to the FsManager, BlockManager, and
DataDirManager with respect to initialization.
- BlockManagers no longer have a Create() function
- BlockManager's dtor will now just wait for DataDir closures to finish
instead of shutting down the DataDirManager as to not directly affect
the DataDirManager
- DataDirManagers now have two static constructors: one to open an
existing layout, and another to create and open a new layout
- FsManagers will only create the BlockManager when opening an fs layout
- A validator is added to check the value of FLAGS_block_manager
before any of the above initialization can occur
Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
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/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
16 files changed, 471 insertions(+), 325 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/7602/8
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] separate DataDirManager from BlockManagers
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/7602
to look at the new patch set (#9).
Change subject: separate DataDirManager from BlockManagers
......................................................................
separate DataDirManager from BlockManagers
Currently, the DataDirManager is owned by the BlockManagers. Since only
blocks are placed in 'data dirs' (i.e. subdirectories named 'data' under
the roots specified by 'fs_data_dirs'), this hierarchy made sense.
However, it would be nice to track other files that fall within
'fs_data_dirs' (e.g. tablet-metadata, consensus-metadata). Splitting the
directory manager from the block manager makes this more feasible.
This patch introduces a number of changes to the FsManager,
BlockManager, and DataDirManager with respect to initialization.
- BlockManagers no longer have a Create() function
- BlockManager's dtor will now just wait for DataDir closures to finish
instead of shutting down the DataDirManager as to not directly affect
the DataDirManager
- DataDirManagers now have two static constructors: one to open an
existing layout, and another to create and open a new layout
- FsManagers will only create the BlockManager when opening an fs layout
- FsManagers will only Open() a DataDirManager if one has not already
been constructed
- A validator is added to check the value of FLAGS_block_manager
before any of the above initialization can occur
Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
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/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
17 files changed, 449 insertions(+), 311 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/7602/9
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
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: Tidy Bot
[kudu-CR] separate DataDirManager from BlockManagers
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/7602
to look at the new patch set (#3).
Change subject: separate DataDirManager from BlockManagers
......................................................................
separate DataDirManager from BlockManagers
Currently, the DataDirManager is owned by the BlockManagers. Since only
blocks are placed in 'data dirs' (i.e. subdirectories named 'data' under
the roots specified by 'fs_data_dirs'), this hierarchy made sense.
However, it would be nice to track other files that fall within
'fs_data_dirs' (e.g. tablet-metadata, consensus-metadata). Splitting the
directory manager from the block manager makes this more feasible.
Some logic previously in the FsManager and BlockManagers is moved into
the DataDirManager:
- canonicalization of data roots now occurs in DataDirManager, ensuring
that the DataDirManager will know about all data directories, even
those that fail to open/canonicalize
- BlockManagers no longer have a Create() function
- BlockManager's dtor will now just wait for DataDir closures to finish
instead of shutting down the DataDirManager as to not directory affect
the DataDirManager
To clarify some vocabulary:
- Root: a top-level directory specified by 'fs_data_dirs'
- Data (root) dir: a subdirectory of a data root, named 'data'. Blocks
are placed in this directory.
Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
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/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
16 files changed, 457 insertions(+), 308 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/7602/3
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] separate DataDirManager from BlockManagers
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: separate DataDirManager from BlockManagers
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7602/1/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
Line 274: const scoped_refptr<MetricEntity>& metric_entity,
> warning: the parameter 'metric_entity' is copied for each invocation but on
Done
http://gerrit.cloudera.org:8080/#/c/7602/1/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
Line 195: static const int kFlagCreateTestHolePunch = 0x1;
> warning: invalid case style for global constant 'FLAG_CREATE_TEST_HOLE_PUNC
Done
Line 196: static const int kFlagCreateFsync = 0x2;
> warning: invalid case style for global constant 'FLAG_CREATE_FSYNC' [readab
Done
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] separate DataDirManager from BlockManagers
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: separate DataDirManager from BlockManagers
......................................................................
Patch Set 6:
(34 comments)
http://gerrit.cloudera.org:8080/#/c/7602/6//COMMIT_MSG
Commit Message:
PS6, Line 23: directory
> directly
Done
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:
PS6, Line 205: // Pass in a report to prevent the block manager from logging
: // unnecessarily.
: FsReport report;
> Nit: move this down to just below Open().
Done
Line 691: ASSERT_OK(env_util::CreateDirIfMissing(this->env_, paths[i]));
> Why not a simple CreateDir() here and below? Under what circumstances would
These shouldn't have been created so it should be safe to just create.
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
PS6, Line 260: // Exposes the underlying DataDirManager.
: virtual DataDirManager* dd_manager() = 0;
> Why does this still exist? Shouldn't callers outside the block managers get
Had been used in tests, but those tests can use the dd_manager directly; removed.
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/data_dirs-test.cc
File src/kudu/fs/data_dirs-test.cc:
Line 65: LOG(INFO) << GetDirNames(kNumDirs)[0];
> Still want this?
Done
Line 77: CHECK_OK(env_util::CreateDirIfMissing(env_, ret[i]));
> Why not a simpler CreateDir?
Done
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
Line 90: TAG_FLAG(fs_lock_data_dirs, unsafe);
> Also tag as 'evolving', for completeness.
Done
Line 272: const char* kInvalidPath = "";
> Where is this used?
Future patch. Eventually we'll need something stored when the directories fail to canonicalize. This isn't it, but it was a precursor to what's implemented later on. Removed.
PS6, Line 288: if (metric_entity) {
: metrics_.reset(new DataDirMetrics(metric_entity));
: }
> You changed metric_entity to be pass-by-cref because it's not being moved h
Done
Line 334: data_root_map_.swap(data_root_map);
> Maybe write to the member at the end of the function, after all the interme
Done
Line 340: if (!canonicalized_data_fs_root.empty()) {
> When is this empty?
Here and elsewhere you saw traces of an implementation where things that failed to canonicalize were given an invalid "" path name. This has changed, and this should be removed (particularly since this patch is independent of disk failures).
Line 372: // Ensure the data dirs exist and create the instance files.
> IMHO this comment was better placed just before the for loop, so it's clear
Done
Line 375: // In test, this is the input path, IRL, this is the paths with kDataDirName
> Please reword; not exactly sure what "input path" means, this should be mor
Yeah, this is a pretty dumb comment and shouldn't have been in this patch.
Line 526: group_by_tablet_map_.clear();
> Isn't this empty at Open() time anyway? Why bother?
In tests, I found it convenient to have calls to Open() be idempotent, i.e. calling Open() multiple times would create leave the directory manager in the same state each time. Given the reset behavior of Init(), this should always be the case even without this.
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
PS6, Line 195: static const int kFlagCreateTestHolePunch = 0x1;
: static const int kFlagCreateFsync = 0x2;
> These were only used in Create(), so they needn't exist at all anymore.
Done
Line 198: static const char* kDataDirName;
> Why does this need to be public? Doc it.
Was only used publicly in tests. Made private.
Line 220: AccessMode mode = AccessMode::READ_WRITE);
> Can we get by without the default value? There aren't that many constructor
Done.
It's used in a handful of tests, but we needn't make that an issue in production code.
Line 240: // max allowed, or if 'mode' is MANDATORY and locks could not be taken.
> 'mode' refers to a parameter that no longer exists.
Done
PS6, Line 229: // Initializes, sanitizes, and canonicalizes the directories.
: Status Init();
:
: // Initializes the data directories on disk.
: //
: // Returns an error if initialized directories already exist.
: Status Create();
:
: // Opens existing data roots from disk and indexes the files found.
: //
: // Returns an error if the number of on-disk directories found exceeds the
: // max allowed, or if 'mode' is MANDATORY and locks could not be taken.
: Status Open();
> Normally we try to provide at most one, sometimes two functions with which
I whittled it down to Init(), Create(), and Open(). The logic in Init() (canonicalization of directories) must always be run first, so Init() is really just a replacement for the public constructor.
PS6, Line 312: std::vector<std::string> GetDataRoots() const;
:
: // Return a list of data directory names.
: std::vector<std::string> GetDataRootDirs() const;
> How about "GetDataRoots" and "GetCanonicalizedDataRoots" to make the distin
The difference here isn't the canonicalization, but rather that GetDataRoots() returns the roots (i.e. those missing the kDataDirName suffix). I've changed it to GetDataDirs() and GetDataRoots().
PS6, Line 371: // Input directories verbatim from the user input fs_data_dirs.
: const std::vector<std::string> data_fs_roots_;
> Style nit: members set by the constructor (including const members) should
Done
PS6, Line 380: fs_data_dirs
> Nit: let's not refer to this gflag by name (since a DataDirManager can be c
Removed this map since it's not unnecessary.
Line 382: RootMap data_root_map_;
> When reading through the code, it was tough to differentiate between data_f
That's fair. The only non-canonicalized strings are data_fs_roots_, which I'm taking out of the DataDirManager. The original intent was to give the directory manager access to the user-input directories so it could canonicalize them and keep track of failed canonicalization. This isn't particularly necessary given my next patch, which just prepends failed directories with a non-sanitized string.
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/file_block_manager.cc
File src/kudu/fs/file_block_manager.cc:
PS6, Line 686: uint16_t uuid_idx = internal::FileBlockLocation::GetDataDirIdx(block_id);
: if (PREDICT_FALSE(dd_manager_->IsDataDirFailed(uuid_idx))) {
: return Status::OK();
: }
> What's this doing here?
Done
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/file_block_manager.h
File src/kudu/fs/file_block_manager.h:
Line 72: static std::string BlockManagerType();
> Where is this used?
It's used in parameterized block manager tests to create the dd_manager. I've since changed the implementation so this is unnecessary.
PS6, Line 74: 'env', 'dd_manager', and 'error_manager'
> Nit: maybe condense with something like "all objects passed by pointer"
Done
Line 121: // Manages and owns all of the block manager's data directories.
> Should probably be reworded given the new relationship and responsibility s
Done
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
PS6, Line 185: // Sanitize and canonicalize the wal root.
: string canonicalized;
: RETURN_NOT_OK_PREPEND(FsManager::SanitizePath(wal_fs_root_),
: "Failed to sanitize Write-Ahead-Log directory (fs_wal_dir)");
: RETURN_NOT_OK_PREPEND(env_->Canonicalize(DirName(wal_fs_root_), &canonicalized),
: Substitute("Failed to canonicalize $0", DirName(wal_fs_root_)));
: canonicalized_wal_fs_root_ = JoinPathSegments(canonicalized, BaseName(wal_fs_root_));
> Is this the same as the logic in data_dirs.cc? Maybe it can all be incorpor
I'd wanted to keep the logic of SanitizePath separate since it's inherently static and is useful in error checking (e.g. a faulty path can be "injected" by prepending with a prefix with spaces, as an easy indicator that canonicalization has failed).
Regardless, having a full Canonicalize method seems reasonable.
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:
Line 217: static Status SanitizePath(const std::string& path);
> Needs doc.
Done
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
PS6, Line 89: // The directory manager must outlive the block manager. Destroy the block
: // manager first to enforce this.
> This seems like a misplaced comment; there's no destroying of either manage
You're right, belongs in ReopenBlockManager.
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
Line 168: static std::string BlockManagerType();
> Where is this used?
It's used to help certain parameterized tests that require the block name. There's a way to do this without anything in prod code.
PS6, Line 169: 'env', 'dd_manager', and 'error_manager'
> Nit: see file_block_manager.h.
Done
Line 361: // Manages and owns all of the block manager's data directories.
> Should probably be reworded given the new relationship and responsibility s
Done
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/util/path_util.cc
File src/kudu/util/path_util.cc:
PS6, Line 56: std::vector<std::string> out;
: for (const std::string& path : v) {
> Remove std:: prefixes.
Done
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] separate DataDirManager from BlockManagers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: separate DataDirManager from BlockManagers
......................................................................
Patch Set 10: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] separate DataDirManager from BlockManagers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: separate DataDirManager from BlockManagers
......................................................................
Patch Set 13: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
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: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] separate DataDirManager from BlockManagers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: separate DataDirManager from BlockManagers
......................................................................
Patch Set 6:
(34 comments)
http://gerrit.cloudera.org:8080/#/c/7602/6//COMMIT_MSG
Commit Message:
PS6, Line 23: directory
directly
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:
PS6, Line 205: // Pass in a report to prevent the block manager from logging
: // unnecessarily.
: FsReport report;
Nit: move this down to just below Open().
Line 691: ASSERT_OK(env_util::CreateDirIfMissing(this->env_, paths[i]));
Why not a simple CreateDir() here and below? Under what circumstances would this path already be created?
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
PS6, Line 260: // Exposes the underlying DataDirManager.
: virtual DataDirManager* dd_manager() = 0;
Why does this still exist? Shouldn't callers outside the block managers get the DataDirManager via the FsManager now?
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/data_dirs-test.cc
File src/kudu/fs/data_dirs-test.cc:
Line 65: LOG(INFO) << GetDirNames(kNumDirs)[0];
Still want this?
Line 77: CHECK_OK(env_util::CreateDirIfMissing(env_, ret[i]));
Why not a simpler CreateDir?
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
Line 90: TAG_FLAG(fs_lock_data_dirs, unsafe);
Also tag as 'evolving', for completeness.
Line 272: const char* kInvalidPath = "";
Where is this used?
PS6, Line 288: if (metric_entity) {
: metrics_.reset(new DataDirMetrics(metric_entity));
: }
You changed metric_entity to be pass-by-cref because it's not being moved here, right? How about adding an std::move here instead?
Line 334: data_root_map_.swap(data_root_map);
Maybe write to the member at the end of the function, after all the intermediate state has been assembled?
Line 340: if (!canonicalized_data_fs_root.empty()) {
When is this empty?
Line 372: // Ensure the data dirs exist and create the instance files.
IMHO this comment was better placed just before the for loop, so it's clear that it applies to the _entirety_ of the loop rather than the first few lines.
Line 375: // In test, this is the input path, IRL, this is the paths with kDataDirName
Please reword; not exactly sure what "input path" means, this should be more specific regarding kDataDirName (i.e. paths with it _appended_), and should say "In production" rather than IRL.
Line 526: group_by_tablet_map_.clear();
Isn't this empty at Open() time anyway? Why bother?
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
PS6, Line 195: static const int kFlagCreateTestHolePunch = 0x1;
: static const int kFlagCreateFsync = 0x2;
These were only used in Create(), so they needn't exist at all anymore.
Line 198: static const char* kDataDirName;
Why does this need to be public? Doc it.
Line 220: AccessMode mode = AccessMode::READ_WRITE);
Can we get by without the default value? There aren't that many constructor calls, are there?
Line 240: // max allowed, or if 'mode' is MANDATORY and locks could not be taken.
'mode' refers to a parameter that no longer exists.
PS6, Line 229: // Initializes, sanitizes, and canonicalizes the directories.
: Status Init();
:
: // Initializes the data directories on disk.
: //
: // Returns an error if initialized directories already exist.
: Status Create();
:
: // Opens existing data roots from disk and indexes the files found.
: //
: // Returns an error if the number of on-disk directories found exceeds the
: // max allowed, or if 'mode' is MANDATORY and locks could not be taken.
: Status Open();
Normally we try to provide at most one, sometimes two functions with which to construct an object:
1. Just one: public constructor. For simple objects that can be either stack or heap allocated. Initialization cannot fail (since constructors do not return anything and our style guide forbids throwing exceptions).
2. Two: constructor and separate initializer. Sometimes the constructor is private, in which case the initializer is static and returns a heap allocated object. The initializer usually returns Status to indicate failures. We use this pattern ("two-phase initialization") when something in initialization may fail and we want to expose that.
In rare cases, we apply pattern #2 with two different branches: one branch for creating something new on disk, and another for loading an existing something from disk.
All this is to say: a public constructor, Init(), Create(), and Open() seems like overkill for DataDirManager. Given that it's a singleton in most cases, you should be able to whittle this down to two public methods: CreateNew() and OpenExisting(). You can make the constructor private.
PS6, Line 312: std::vector<std::string> GetDataRoots() const;
:
: // Return a list of data directory names.
: std::vector<std::string> GetDataRootDirs() const;
How about "GetDataRoots" and "GetCanonicalizedDataRoots" to make the distinction more clear?
PS6, Line 371: // Input directories verbatim from the user input fs_data_dirs.
: const std::vector<std::string> data_fs_roots_;
Style nit: members set by the constructor (including const members) should precede other members. That is, this should join mode_, env_, and block_manager_type_.
PS6, Line 380: fs_data_dirs
Nit: let's not refer to this gflag by name (since a DataDirManager can be constructed with an arbitrary value for 'data_roots'). Just refer back to the verbatim representation provided at construction time.
Line 382: RootMap data_root_map_;
When reading through the code, it was tough to differentiate between data_fs_roots_ and data_root_map_, especially with respect to canonicalization. Perhaps change this to "canonicalized_data_fs_root_map_"?
Alternatively, use an std::map (which retains insertion order I believe) and do away with canonicalized_data_fs_roots_ altogether.
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/file_block_manager.cc
File src/kudu/fs/file_block_manager.cc:
PS6, Line 686: uint16_t uuid_idx = internal::FileBlockLocation::GetDataDirIdx(block_id);
: if (PREDICT_FALSE(dd_manager_->IsDataDirFailed(uuid_idx))) {
: return Status::OK();
: }
What's this doing here?
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/file_block_manager.h
File src/kudu/fs/file_block_manager.h:
Line 72: static std::string BlockManagerType();
Where is this used?
PS6, Line 74: 'env', 'dd_manager', and 'error_manager'
Nit: maybe condense with something like "all objects passed by pointer"
Line 121: // Manages and owns all of the block manager's data directories.
Should probably be reworded given the new relationship and responsibility structure.
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
PS6, Line 185: // Sanitize and canonicalize the wal root.
: string canonicalized;
: RETURN_NOT_OK_PREPEND(FsManager::SanitizePath(wal_fs_root_),
: "Failed to sanitize Write-Ahead-Log directory (fs_wal_dir)");
: RETURN_NOT_OK_PREPEND(env_->Canonicalize(DirName(wal_fs_root_), &canonicalized),
: Substitute("Failed to canonicalize $0", DirName(wal_fs_root_)));
: canonicalized_wal_fs_root_ = JoinPathSegments(canonicalized, BaseName(wal_fs_root_));
Is this the same as the logic in data_dirs.cc? Maybe it can all be incorporated into FsManager::SanitizePath?
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:
Line 217: static Status SanitizePath(const std::string& path);
Needs doc.
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
PS6, Line 89: // The directory manager must outlive the block manager. Destroy the block
: // manager first to enforce this.
This seems like a misplaced comment; there's no destroying of either manager here.
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
Line 168: static std::string BlockManagerType();
Where is this used?
PS6, Line 169: 'env', 'dd_manager', and 'error_manager'
Nit: see file_block_manager.h.
Line 361: // Manages and owns all of the block manager's data directories.
Should probably be reworded given the new relationship and responsibility structure.
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/util/path_util.cc
File src/kudu/util/path_util.cc:
PS6, Line 56: std::vector<std::string> out;
: for (const std::string& path : v) {
Remove std:: prefixes.
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] separate DataDirManager from BlockManagers
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/7602
to look at the new patch set (#7).
Change subject: separate DataDirManager from BlockManagers
......................................................................
separate DataDirManager from BlockManagers
Currently, the DataDirManager is owned by the BlockManagers. Since only
blocks are placed in 'data dirs' (i.e. subdirectories named 'data' under
the roots specified by 'fs_data_dirs'), this hierarchy made sense.
However, it would be nice to track other files that fall within
'fs_data_dirs' (e.g. tablet-metadata, consensus-metadata). Splitting the
directory manager from the block manager makes this more feasible.
Some logic previously in the FsManager and BlockManagers is moved into
the DataDirManager:
- canonicalization of data roots now occurs in DataDirManager's new
public initializer Init().
- BlockManagers no longer have a Create() function
- BlockManager's dtor will now just wait for DataDir closures to finish
instead of shutting down the DataDirManager as to not directly affect
the DataDirManager
To clarify some vocabulary:
- Data root: a top-level directory specified by 'fs_data_dirs'
- Data dir: a subdirectory of a data root, named 'data'. Blocks are
placed in this directory.
Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
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/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
16 files changed, 473 insertions(+), 349 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/7602/7
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
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: Tidy Bot
[kudu-CR] separate DataDirManager from BlockManagers
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/7602
to look at the new patch set (#6).
Change subject: separate DataDirManager from BlockManagers
......................................................................
separate DataDirManager from BlockManagers
Currently, the DataDirManager is owned by the BlockManagers. Since only
blocks are placed in 'data dirs' (i.e. subdirectories named 'data' under
the roots specified by 'fs_data_dirs'), this hierarchy made sense.
However, it would be nice to track other files that fall within
'fs_data_dirs' (e.g. tablet-metadata, consensus-metadata). Splitting the
directory manager from the block manager makes this more feasible.
Some logic previously in the FsManager and BlockManagers is moved into
the DataDirManager:
- canonicalization of data roots now occurs in DataDirManager. This will
be useful, as it will enable that the DataDirManager can know all data
directories, even those that fail to open/canonicalize
- BlockManagers no longer have a Create() function
- BlockManager's dtor will now just wait for DataDir closures to finish
instead of shutting down the DataDirManager as to not directory affect
the DataDirManager
To clarify some vocabulary:
- Root: a top-level directory specified by 'fs_data_dirs'
- Data (root) dir: a subdirectory of a data root, named 'data'. Blocks
are placed in this directory.
Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
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/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
16 files changed, 459 insertions(+), 310 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/7602/6
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] separate DataDirManager from BlockManagers
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: separate DataDirManager from BlockManagers
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7602/9/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
Line 381: metadata_.swap(metadata);
> Why is this (and the other changes to 'metadata' in this function) necessar
Ah, I started to split out the behavior of FsManager::CreateInit.. or Open to be only called once. Not needed anymore.
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
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: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] separate DataDirManager from BlockManagers
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: separate DataDirManager from BlockManagers
......................................................................
Patch Set 7:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/7602/7/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:
Line 146: bm_.reset();
> Why does the block manager have to be destroyed separately up here? Why isn
Done
http://gerrit.cloudera.org:8080/#/c/7602/7/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
Line 289: LOG(INFO) << "Constructing a new DataDirManager";
> Still need this?
Done
Line 292: metrics_.reset(new DataDirMetrics(std::move(opts.metric_entity)));
> Ah, I see, this is because DataDirMetrics takes the MetricEntity by cref. A
Done
Line 292: metrics_.reset(new DataDirMetrics(std::move(opts.metric_entity)));
> warning: passing result of std::move() as a const reference argument; no mo
Done
PS7, Line 339: canonicalized_data_roots.insert(canonicalized_data_roots.end(),
: canonicalized_data_roots_set.begin(),
: canonicalized_data_roots_set.end());
> Isn't this logically the same as the returned values from the calls to FsM
Not quite, it removes duplicates, but in any case, I've taken this out and moved it back to the FsManager.
Line 344: dd_manager->reset(new DataDirManager(env, opts, canonicalized_data_roots));
> warning: parameter 'opts' is passed by value and only copied once; consider
Done
PS7, Line 350: const int kFlagCreateTestHolePunch = 0x1;
: const int kFlagCreateFsync = 0x2;
> But why bother with flags at all? Flags are a means of defining an API and
Ah, makes sense, way clearer. Done.
http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
PS6, Line 229: // Shuts down all directories' thread pools.
: void Shutdown();
:
: // Waits on all directories' thread pools.
: void WaitOnClosures();
:
: // Initializes the data directories on disk.
: //
: // Returns an error if initialized directories already exist.
: Status Create();
:
: // Opens existing data roots from disk and indexes the files found.
: //
> Would it be possible to convert the static Init and the Create and/or Open
Done. It was mainly a matter of ordering of canonicalization. I.e. the FsManager needs the canonicalized roots before doing anything; I thought I could push some of it to the directory manager, but I think to do a complete job on that might require making the WAL dir known to the directory manager too.
Originally I'd wanted to have the directory manager handle the canonicalization failures, but I realize we might be able to get by doing it at the FsManager level (e.g. by prepending an unsanitized prefix to the non-canonicalized path).
In any case, I've taken out the canonicalization changes since I don't think they're necessary for this patch, and probably won't be for handling failures at startup.
http://gerrit.cloudera.org:8080/#/c/7602/7/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
PS7, Line 200: Defaultas to fales
> Defaults to false
Done
Line 351: DataDirManager(Env* env,
> warning: function 'kudu::fs::DataDirManager::DataDirManager' has a definiti
Done
Line 378: // The canonicalized roots provided at construction time, taken verbatim.
> I understand what this means, but "construction time" is a little weird bec
Done. Ah interesting point; how about, "provided to the constructor"
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
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: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] separate DataDirManager from BlockManagers
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/7602
to look at the new patch set (#2).
Change subject: separate DataDirManager from BlockManagers
......................................................................
separate DataDirManager from BlockManagers
Currently, the DataDirManager is owned by the BlockManagers. Since only
blocks are placed in 'data dirs' (i.e. subdirectories named 'data' under
the roots specified by 'fs_data_dirs'), this hierarchy made sense.
However, it would be nice to track other files that fall within
'fs_data_dirs' (e.g. tablet-metadata, consensus-metadata). Splitting the
directory manager from the block manager makes this more feasible.
Some logic previously in the FsManager and BlockManagers is moved into
the DataDirManager:
- canonicalization of data roots now occurs in DataDirManager, ensuring
that the DataDirManager will know about all data directories, even
those that fail to open/canonicalize
- BlockManagers no longer have a Create() function
- BlockManager's dtor will now just wait for DataDir closures to finish
instead of shutting down the DataDirManager as to not directory affect
the DataDirManager
To clarify some vocabulary:
- Root: a top-level directory specified by 'fs_data_dirs'
- Data (root) dir: a subdirectory of a data root, named 'data'. Blocks
are placed in this directory.
Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
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/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
16 files changed, 435 insertions(+), 284 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/7602/2
--
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot