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), &dd_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 <a...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Fri, 10 Nov 2017 01:02:00 +0000 Gerrit-HasComments: Yes
