You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2016/10/22 01:41:25 UTC
[kudu-CR] block manager: consolidate data directory management
Hello Mike Percy, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4793
to review the following change.
Change subject: block_manager: consolidate data directory management
......................................................................
block_manager: consolidate data directory management
This patch changes how data directories are handled. All of the duplicated
logic between the two block managers is now encapsulated in two new classes
(DataDir and DataDirManager), which can serve as the future foundation for
a "disk grouping" feature.
There should be little to no functional changes. Of note:
- The change to the LBM block map constructor yields a map with no initial
entries, hence the test change.
- This refactoring makes the LBM check instance file integrity during Open()
for the first time. Given that the remainder of the instance file handling
was identical in both block managers, this should be harmless.
- The LBM now opens per-directory instance files serially. This is a tiny
amount of I/O next to container scanning, and while it can be done
concurrently in the DataDirManager, I don't think it's worth the effort.
Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7
---
M src/kudu/fs/CMakeLists.txt
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.cc
M src/kudu/fs/block_manager.h
A src/kudu/fs/data_dirs.cc
A 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/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
10 files changed, 590 insertions(+), 467 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/4793/1
--
To view, visit http://gerrit.cloudera.org:8080/4793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] block manager: consolidate data directory management
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: block_manager: consolidate data directory management
......................................................................
Patch Set 5: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4793/4/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
Line 1179: return s;
> Correct, we'll let the destructor clean them up. We could explicitly close
Nit: this kinda assumes that container Open() failure is always followed by a destructor call, which isn't a safe assumption IMHO. We can perhaps put a comment here to indicate that.
--
To view, visit http://gerrit.cloudera.org:8080/4793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] block manager: consolidate data directory management
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4793
to look at the new patch set (#4).
Change subject: block_manager: consolidate data directory management
......................................................................
block_manager: consolidate data directory management
This patch changes how data directories are handled. All of the duplicated
logic between the two block managers is now encapsulated in two new classes
(DataDir and DataDirManager), which can serve as the future foundation for
a "disk grouping" feature.
There should be little to no functional changes. Of note:
- This refactoring makes the LBM check instance file integrity during Open()
for the first time. Given that the remainder of the instance file handling
was identical in both block managers, this should be harmless.
- The LBM now opens per-directory instance files serially. This is a tiny
amount of I/O next to container scanning, and while it can be done
concurrently in the DataDirManager, I don't think it's worth the effort.
Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7
---
M src/kudu/fs/CMakeLists.txt
M src/kudu/fs/block_manager.cc
M src/kudu/fs/block_manager.h
A src/kudu/fs/data_dirs.cc
A 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/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
9 files changed, 607 insertions(+), 470 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/4793/4
--
To view, visit http://gerrit.cloudera.org:8080/4793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] block manager: consolidate data directory management
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: block_manager: consolidate data directory management
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/4793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] block manager: consolidate data directory management
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: block_manager: consolidate data directory management
......................................................................
Patch Set 4:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/4793/4/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
PS4, Line 73: reconfigure "
: "Kudu with --block_manager=file
> At this point, 'reconfigure' means starting the kudu with a new fs_root_pat
This is logged from CheckHolePunch(), called via Create(). In both cases, we take care to delete intermediate files in the event of an error. The idea is that if you see this, you really could change your Kudu configuration to use --block_manager=file, restart, and go on your merry way. You don't need to reformat anything.
Line 250: LOG(WARNING) << "Proceeding without lock";
> This may be a relocated code, but I am curious the instance->LockFile() may
Under the hood, Lock() calls open() and fcntl(). We expect open() to succeed because we just called open() via LoadFromDisk() a few lines earlier. So that really just leaves fcntl().
Separately, I think it's reasonable that if you asked for LockMode::OPTIONAL, you get what you paid for (i.e. continuing in spite of any error). If there's something truly wrong, we'll probably fail during an I/O later on.
http://gerrit.cloudera.org:8080/#/c/4793/4/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
Line 77: class DataDirManager {
> Can we put one or two liner description for this to make it clear how this
Done, though I tried to stay away from a "this is how it can be used" explanation since that sort of thing tends to get stale.
http://gerrit.cloudera.org:8080/#/c/4793/4/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
Line 170: class LogBlockContainer {
> A basic C++ question not related to this change - was it intentional to put
Yes, this is because LogBlockContainer is private to the LogBlockManager, so there's no reason to expose it to users of the LogBlockManager.
PS4, Line 1153: OPTIONAL
> Just a brainstorm idea here(not suggesting for this patch): I wonder if we
The OPTIONAL/MANDATORY duality here is orthogonal to UNIX advisory/mandatory locking. Under the hood, all of the locking is advisory (see env_posix.cc). Here, MANDATORY means "a lock failure is a fatal failure" and OPTIONAL means "proceed despite a lock failure".
Line 1179: return s;
> Do we need to worry about cleaning up successfully opened data_dirs here ?
Correct, we'll let the destructor clean them up. We could explicitly close them ourselves, but it's not strictly necessary.
http://gerrit.cloudera.org:8080/#/c/4793/4/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
Line 308: DataDirManager data_dirs_;
> Nit: can we slightly modify the variable name to reflect that it's a manage
Done
--
To view, visit http://gerrit.cloudera.org:8080/4793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] block manager: consolidate data directory management
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: block_manager: consolidate data directory management
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] block manager: consolidate data directory management
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: block_manager: consolidate data directory management
......................................................................
Patch Set 2:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/4793/2/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
PS2, Line 310: do {
: cur_idx = data_dirs_next_.Load();
: next_idx = (cur_idx + 1) % data_dirs_.size();
: } while (!data_dirs_next_.CompareAndSet(cur_idx, next_idx));
:
:
since there's no longer a check against "full paths" here, I think you could just use Increment() and always mod the result by data_dirs_.size(), instead of reimplementing increment via CAS
Line 318: DataDir* DataDirManager::FindDataDirByUuidIndex(uint16_t uuid_idx) const {
any kind of CHECK you can do to make sure it's already opened?
Line 323: return FindCopy(uuid_idx_by_data_dir_, dir, uuid_idx);
here too
http://gerrit.cloudera.org:8080/#/c/4793/2/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
Line 93: // Initializes the data directories on disk.
can you specify what happens if they already exist?
Line 94: Status Create(bool test_hole_punch, bool fsync);
would a flags set be a more readable choice here?
Line 97: Status Open(int max_data_dirs, LockMode mode);
doc 'max_data_dirs'
PS2, Line 103: uuid index,
maybe a call-out here to where the idea of 'uuid index' is defined? I don't remember what this term means.
http://gerrit.cloudera.org:8080/#/c/4793/2/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
PS2, Line 241: full_root_paths
rename this and update docs
Line 285: // Success or failure is set in 'result_status'.
why not return Status?
--
To view, visit http://gerrit.cloudera.org:8080/4793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] block manager: consolidate data directory management
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: block_manager: consolidate data directory management
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4793/2/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
PS2, Line 310: do {
: cur_idx = data_dirs_next_.Load();
: next_idx = (cur_idx + 1) % data_dirs_.size();
: } while (!data_dirs_next_.CompareAndSet(cur_idx, next_idx));
:
:
> Done
On second thought, the next patch restores the full path checking, which could cause data_dirs_next_ to increase at a faster rate.
So I'll leave it the way it is.
--
To view, visit http://gerrit.cloudera.org:8080/4793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] block manager: consolidate data directory management
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: block_manager: consolidate data directory management
......................................................................
Patch Set 2:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/4793/2/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
PS2, Line 310: do {
: cur_idx = data_dirs_next_.Load();
: next_idx = (cur_idx + 1) % data_dirs_.size();
: } while (!data_dirs_next_.CompareAndSet(cur_idx, next_idx));
:
:
> since there's no longer a check against "full paths" here, I think you coul
Done
Line 318: DataDir* DataDirManager::FindDataDirByUuidIndex(uint16_t uuid_idx) const {
> any kind of CHECK you can do to make sure it's already opened?
DataDirs only exist after Open() is called. Create() only creates data directories on disk.
Line 323: return FindCopy(uuid_idx_by_data_dir_, dir, uuid_idx);
> here too
See above.
http://gerrit.cloudera.org:8080/#/c/4793/2/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
Line 93: // Initializes the data directories on disk.
> can you specify what happens if they already exist?
Sure (it'll fail).
Line 94: Status Create(bool test_hole_punch, bool fsync);
> would a flags set be a more readable choice here?
Sure.
Line 97: Status Open(int max_data_dirs, LockMode mode);
> doc 'max_data_dirs'
Done
PS2, Line 103: uuid index,
> maybe a call-out here to where the idea of 'uuid index' is defined? I don't
Sure (fs.proto).
http://gerrit.cloudera.org:8080/#/c/4793/2/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
PS2, Line 241: full_root_paths
> rename this and update docs
The existing name/docs aren't wrong; it's OK (if perhaps imprecise) to think of data directories as "root paths". I left it as-is largely because I blew all of this away in the next patch anyway.
Line 285: // Success or failure is set in 'result_status'.
> why not return Status?
Because OpenDataDir() is run on a thread pool as a closure of fixed type. It's not a lambda so capturing isn't an option, so instead we pass the status as an OUT parameter.
--
To view, visit http://gerrit.cloudera.org:8080/4793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] block manager: consolidate data directory management
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Todd Lipcon, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4793
to look at the new patch set (#5).
Change subject: block_manager: consolidate data directory management
......................................................................
block_manager: consolidate data directory management
This patch changes how data directories are handled. All of the duplicated
logic between the two block managers is now encapsulated in two new classes
(DataDir and DataDirManager), which can serve as the future foundation for
a "disk grouping" feature.
There should be little to no functional changes. Of note:
- This refactoring makes the LBM check instance file integrity during Open()
for the first time. Given that the remainder of the instance file handling
was identical in both block managers, this should be harmless.
- The LBM now opens per-directory instance files serially. This is a tiny
amount of I/O next to container scanning, and while it can be done
concurrently in the DataDirManager, I don't think it's worth the effort.
Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7
---
M src/kudu/fs/CMakeLists.txt
M src/kudu/fs/block_manager.cc
M src/kudu/fs/block_manager.h
A src/kudu/fs/data_dirs.cc
A 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/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
9 files changed, 609 insertions(+), 470 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/4793/5
--
To view, visit http://gerrit.cloudera.org:8080/4793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] block manager: consolidate data directory management
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: block_manager: consolidate data directory management
......................................................................
block_manager: consolidate data directory management
This patch changes how data directories are handled. All of the duplicated
logic between the two block managers is now encapsulated in two new classes
(DataDir and DataDirManager), which can serve as the future foundation for
a "disk grouping" feature.
There should be little to no functional changes. Of note:
- This refactoring makes the LBM check instance file integrity during Open()
for the first time. Given that the remainder of the instance file handling
was identical in both block managers, this should be harmless.
- The LBM now opens per-directory instance files serially. This is a tiny
amount of I/O next to container scanning, and while it can be done
concurrently in the DataDirManager, I don't think it's worth the effort.
Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7
Reviewed-on: http://gerrit.cloudera.org:8080/4793
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
Reviewed-by: Dinesh Bhat <di...@cloudera.com>
---
M src/kudu/fs/CMakeLists.txt
M src/kudu/fs/block_manager.cc
M src/kudu/fs/block_manager.h
A src/kudu/fs/data_dirs.cc
A 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/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
9 files changed, 609 insertions(+), 470 deletions(-)
Approvals:
Dinesh Bhat: Looks good to me, but someone else must approve
Mike Percy: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] block manager: consolidate data directory management
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: block_manager: consolidate data directory management
......................................................................
Patch Set 4:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/4793/4/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
PS4, Line 73: reconfigure "
: "Kudu with --block_manager=file
At this point, 'reconfigure' means starting the kudu with a new fs_root_path or reformatting the fs before restarting the kudu on that node right ? Basically upon holePunchError, we can not restart the kudu-tserver and switch to FBM without following the pre-requisite cleanup steps, right ? Just wanted to make sure this error message is unambiguous to the log reader.
Line 250: LOG(WARNING) << "Proceeding without lock";
This may be a relocated code, but I am curious the instance->LockFile() may have failed for number of reasons, and we still proceed without verifying if the file underneath had some other failure than fcntl(lock) related failure. Or may be we expect the file to have been opened by this stage and only failure we expect here is a fcntl(lock) , isn't it ?
http://gerrit.cloudera.org:8080/#/c/4793/4/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
Line 77: class DataDirManager {
Can we put one or two liner description for this to make it clear how this cab be leveraged by FBM and LBM ?
http://gerrit.cloudera.org:8080/#/c/4793/4/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
Line 170: class LogBlockContainer {
A basic C++ question not related to this change - was it intentional to put this class inside .cc instead of log_block_manager.h ? When would we normally do that ?
PS4, Line 1153: OPTIONAL
Just a brainstorm idea here(not suggesting for this patch): I wonder if we could do away with just 2 modes here: one which locks another which doesn't lock. I couldn't trace these modes to advisory vs mandatory locks for the instance files underneath. I may have missed it though.
Line 1179: return s;
Do we need to worry about cleaning up successfully opened data_dirs here ? Or perhaps we bank on LBM destructor to do that cleanup ?
http://gerrit.cloudera.org:8080/#/c/4793/4/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
Line 308: DataDirManager data_dirs_;
Nit: can we slightly modify the variable name to reflect that it's a manager instance and not data_dir instance ? At first look, looking at variable name alone, I thought this is incarnation of DataDir.
--
To view, visit http://gerrit.cloudera.org:8080/4793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] block manager: consolidate data directory management
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4793
to look at the new patch set (#2).
Change subject: block_manager: consolidate data directory management
......................................................................
block_manager: consolidate data directory management
This patch changes how data directories are handled. All of the duplicated
logic between the two block managers is now encapsulated in two new classes
(DataDir and DataDirManager), which can serve as the future foundation for
a "disk grouping" feature.
There should be little to no functional changes. Of note:
- This refactoring makes the LBM check instance file integrity during Open()
for the first time. Given that the remainder of the instance file handling
was identical in both block managers, this should be harmless.
- The LBM now opens per-directory instance files serially. This is a tiny
amount of I/O next to container scanning, and while it can be done
concurrently in the DataDirManager, I don't think it's worth the effort.
Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7
---
M src/kudu/fs/CMakeLists.txt
M src/kudu/fs/block_manager.cc
M src/kudu/fs/block_manager.h
A src/kudu/fs/data_dirs.cc
A 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/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
9 files changed, 587 insertions(+), 460 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/4793/2
--
To view, visit http://gerrit.cloudera.org:8080/4793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] block manager: consolidate data directory management
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: block_manager: consolidate data directory management
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4793/4/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
Line 1179: return s;
> Nit: this kinda assumes that container Open() failure is always followed by
I disagree. Open() failures are fatal; all callers will use RETURN_NOT_OK() to surface them, eventually shutting down the process and cleaning up extant objects like these.
To clean up here, I'd need to make the DataDirManager non-const and defer its construction from the LogBlockManager constructor out to here and to LogBlockManager::Create(), then use swap() to move it into the LBM state only if all opens succeeded. I don't think that's worth the complexity.
A comment explaining this would either be non-visible (if it were here) or too distracting given the utility (if in the function header).
--
To view, visit http://gerrit.cloudera.org:8080/4793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] block manager: consolidate data directory management
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Todd Lipcon, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4793
to look at the new patch set (#3).
Change subject: block_manager: consolidate data directory management
......................................................................
block_manager: consolidate data directory management
This patch changes how data directories are handled. All of the duplicated
logic between the two block managers is now encapsulated in two new classes
(DataDir and DataDirManager), which can serve as the future foundation for
a "disk grouping" feature.
There should be little to no functional changes. Of note:
- This refactoring makes the LBM check instance file integrity during Open()
for the first time. Given that the remainder of the instance file handling
was identical in both block managers, this should be harmless.
- The LBM now opens per-directory instance files serially. This is a tiny
amount of I/O next to container scanning, and while it can be done
concurrently in the DataDirManager, I don't think it's worth the effort.
Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7
---
M src/kudu/fs/CMakeLists.txt
M src/kudu/fs/block_manager.cc
M src/kudu/fs/block_manager.h
A src/kudu/fs/data_dirs.cc
A 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/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
9 files changed, 607 insertions(+), 470 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/4793/3
--
To view, visit http://gerrit.cloudera.org:8080/4793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>