Adar Dembo has posted comments on this change. Change subject: separate DataDirManager from BlockManagers ......................................................................
Patch Set 8: (6 comments) http://gerrit.cloudera.org:8080/#/c/7602/8/src/kudu/fs/block_manager-stress-test.cc File src/kudu/fs/block_manager-stress-test.cc: PS8, Line 144: : : : : : : : Don't we need to preserve this in some capacity? http://gerrit.cloudera.org:8080/#/c/7602/7/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: Line 378: // Goes through the data dirs in 'uuid_indices' and populates > Done. Ah interesting point; how about, "provided to the constructor" Yeah, that's more clear. http://gerrit.cloudera.org:8080/#/c/7602/8/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: PS8, Line 231: qcquired acquired http://gerrit.cloudera.org:8080/#/c/7602/8/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: Line 164 Maybe restore this comment? Line 71: return value == "log" || value == "file"; On macOS can you set this up so that "file" is the only valid option? Line 270: RETURN_NOT_OK(DataDirManager::OpenExisting(env_, Hmm, but in the case of FsManager::CreateNewFileSystemLayout followed by FsManager::Open, we'll end up creating the DataDirManager twice, once in CreateNew() and once in OpenExisting(). It's not the biggest deal I guess, but maybe it's fixable without too much work? -- To view, visit http://gerrit.cloudera.org:8080/7602 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
