[kudu-CR] separate DataDirManager from BlockManagers

[Adar Dembo (Code Review)]

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 
---
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] separate DataDirManager from BlockManagers

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] separate DataDirManager from BlockManagers

[Andrew Wong (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] separate DataDirManager from BlockManagers

[Andrew Wong (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] separate DataDirManager from BlockManagers

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] separate DataDirManager from BlockManagers

[Andrew Wong (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] separate DataDirManager from BlockManagers

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] separate DataDirManager from BlockManagers

[Andrew Wong (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] separate DataDirManager from BlockManagers

[Andrew Wong (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] separate DataDirManager from BlockManagers

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] separate DataDirManager from BlockManagers

[Andrew Wong (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] separate DataDirManager from BlockManagers

[Andrew Wong (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] separate DataDirManager from BlockManagers

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] separate DataDirManager from BlockManagers

[Andrew Wong (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] separate DataDirManager from BlockManagers

[Andrew Wong (Code Review)]

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 

[kudu-CR] separate DataDirManager from BlockManagers

[Adar Dembo (Code Review)]

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 

[kudu-CR] separate DataDirManager from BlockManagers

[Andrew Wong (Code Review)]

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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] separate DataDirManager from BlockManagers

[Andrew Wong (Code Review)]

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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] separate DataDirManager from BlockManagers

[Andrew Wong (Code Review)]

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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] separate DataDirManager from BlockManagers

[Andrew Wong (Code Review)]

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& 
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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] separate DataDirManager from BlockManagers

[Andrew Wong (Code Review)]

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 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot