[kudu-CR] tool: new action for adding to the set of data directories

2017-11-13 Thread Todd Lipcon (Code Review) 
Todd Lipcon has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8352 )

Change subject: tool: new action for adding to the set of data directories
..

tool: new action for adding to the set of data directories

This patch includes support for adding to the set of data directories in
an existing Kudu filesystem. The only user-facing bit is a new FsManager
option that, when set, augments Open() to also look for missing fs
roots. If any are found, they will be created, and the existing data
directory instance files updated to recognize these new roots. Also
included is a new tool action that opens an FsManager with this option
set.

Updating the set of data directories is a complex, multi-step operation,
and a single error could leave the filesystem in a difficult-to-repair
state. As such, there is some fairly gnarly rollback code that attempts
to undo the changes made in the event of an error.

This logic can be extended to remove data directories. This patch only
addresses adding.

Change-Id: I6ddbdd6cc6231996e7802a622a8b4691527a0643
Reviewed-on: http://gerrit.cloudera.org:8080/8352
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/fs/block_manager_util-test.cc
M src/kudu/fs/block_manager_util.cc
M src/kudu/fs/block_manager_util.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/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_fs.cc
11 files changed, 730 insertions(+), 169 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Todd Lipcon: Looks good to me, approved

--
To view, visit http://gerrit.cloudera.org:8080/8352
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6ddbdd6cc6231996e7802a622a8b4691527a0643
Gerrit-Change-Number: 8352
Gerrit-PatchSet: 10
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] tool: new action for adding to the set of data directories

2017-11-13 Thread Todd Lipcon (Code Review) 
Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8352 )

Change subject: tool: new action for adding to the set of data directories
..


Patch Set 9: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/8352
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6ddbdd6cc6231996e7802a622a8b4691527a0643
Gerrit-Change-Number: 8352
Gerrit-PatchSet: 9
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 14 Nov 2017 07:40:32 +
Gerrit-HasComments: No

[kudu-CR] tool: new action for adding to the set of data directories

2017-11-09 Thread Andrew Wong (Code Review) 
Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8352 )

Change subject: tool: new action for adding to the set of data directories
..


Patch Set 9:

(8 comments)

Did a light refactor in DataDirManager::Open() to take out some recursion.

Also updated FsManager::Open() to log the created files/directories.

http://gerrit.cloudera.org:8080/#/c/8352/8//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8352/8//COMMIT_MSG@21
PS8, Line 21:
: This logic can be extended to remove data directories. This patch 
only
: addresses adding.
> 2202 is now addressed. I think it's still fine to split this up to a follow
Done


http://gerrit.cloudera.org:8080/#/c/8352/8/src/kudu/fs/block_manager_util.h
File src/kudu/fs/block_manager_util.h:

http://gerrit.cloudera.org:8080/#/c/8352/8/src/kudu/fs/block_manager_util.h@92
PS8, Line 92:   std::string dir() const { return DirName(filename_); }
:   const std::string& path() const { return filename_; }
> these are somewhat confusingly ambiguous. Maybe we should rename 'path' to
Done


http://gerrit.cloudera.org:8080/#/c/8352/8/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:

http://gerrit.cloudera.org:8080/#/c/8352/8/src/kudu/fs/data_dirs.h@225
PS8, Line 225:   // Defaults to false. If true, 'read_only' must be false.
> include the default in these docs? (or use inline default)
Done


http://gerrit.cloudera.org:8080/#/c/8352/8/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8352/8/src/kudu/fs/fs_manager.cc@372
PS8, Line 372:   canonicalized_data_fs_roots_, std::move(dm_opts), 
_manager_));
> put up a patch here to fix this false positive: http://gerrit.cloudera.org:
Ack


http://gerrit.cloudera.org:8080/#/c/8352/8/src/kudu/fs/fs_manager.cc@433
PS8, Line 433: // Delete directories in reverse order since parent 
directories will have
nit: This is confusing because they get synced at L466.
maybe "...will not be synchronized to disk until the directory manager is 
created" or something?


http://gerrit.cloudera.org:8080/#/c/8352/8/src/kudu/fs/fs_manager.cc@469
PS8, Line 469:
nit: this is a bit confusing because they're not being deleted. Rename to 
'new_dirs' and 'new_files'?


http://gerrit.cloudera.org:8080/#/c/8352/8/src/kudu/fs/fs_manager.cc@508
PS8, Line 508:   return Status::AlreadyPresent(
 :   Substitute("FSManager root is not empty. See $0", 
KuduDocsTroubleshootingUrl()),
 :  
Not your fault, but this shouldn't be possible.


http://gerrit.cloudera.org:8080/#/c/8352/8/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/8352/8/src/kudu/tools/kudu-tool-test.cc@409
PS8, Line 409: update
> let's be more specific here like update_dirs?
Done



--
To view, visit http://gerrit.cloudera.org:8080/8352
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6ddbdd6cc6231996e7802a622a8b4691527a0643
Gerrit-Change-Number: 8352
Gerrit-PatchSet: 9
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 10 Nov 2017 01:02:00 +
Gerrit-HasComments: Yes

[kudu-CR] tool: new action for adding to the set of data directories

2017-11-09 Thread Andrew Wong (Code Review) 
Andrew Wong has uploaded a new patch set (#9) to the change originally created 
by Adar Dembo. ( http://gerrit.cloudera.org:8080/8352 )

Change subject: tool: new action for adding to the set of data directories
..

tool: new action for adding to the set of data directories

This patch includes support for adding to the set of data directories in
an existing Kudu filesystem. The only user-facing bit is a new FsManager
option that, when set, augments Open() to also look for missing fs
roots. If any are found, they will be created, and the existing data
directory instance files updated to recognize these new roots. Also
included is a new tool action that opens an FsManager with this option
set.

Updating the set of data directories is a complex, multi-step operation,
and a single error could leave the filesystem in a difficult-to-repair
state. As such, there is some fairly gnarly rollback code that attempts
to undo the changes made in the event of an error.

This logic can be extended to remove data directories. This patch only
addresses adding.

Change-Id: I6ddbdd6cc6231996e7802a622a8b4691527a0643
---
M src/kudu/fs/block_manager_util-test.cc
M src/kudu/fs/block_manager_util.cc
M src/kudu/fs/block_manager_util.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/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_fs.cc
11 files changed, 730 insertions(+), 169 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/8352/9
--
To view, visit http://gerrit.cloudera.org:8080/8352
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6ddbdd6cc6231996e7802a622a8b4691527a0643
Gerrit-Change-Number: 8352
Gerrit-PatchSet: 9
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon