Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 )
Change subject: KUDU-1489: allow configuration of metadata dir ...................................................................... Patch Set 6: (18 comments) http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG@20 PS4, Line 20: It is up to the user to take caution in changing this flag; updating the : flag without also manually moving any existing metadata will cause Kudu : to fail at startup. > Isn't the same true of fs_wal_dir and fs_data_dirs? Or is changing fs_metad Good point. No point in bringing it up, I'll take this out. http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG@29 PS4, Line 29: cases > Nit: case Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/block_manager_util.cc File src/kudu/fs/block_manager_util.cc: http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/block_manager_util.cc@176 PS4, Line 176: DCHECK_NE(first_healthy, -1); // Guaranteed by DataDirManager::LoadInstnaces(). > Can we not depend on this, and instead return a bad status if there are no Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc File src/kudu/fs/fs_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@258 PS4, Line 258: env_->DeleteRecursively(GetTestPath("wal")); : env_->DeleteRecursively(GetTestPath("data")); > Maybe use different tests so you needn't clean up in between? Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@271 PS4, Line 271: opts.metadata_root = ""; > Maybe opts.metadata_root.clear() would be more idiomatic? Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@284 PS4, Line 284: ASSERT_OK(fs_manager()->Open()); > After this could you check that the WAL root and the metadata root aren't t Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@288 PS4, Line 288: opts.metadata_root = ""; > .clear() here too. Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@517 PS4, Line 517: // Try to open with a new data dir at the front of the list. > Nit: the other "Try to ..." comments here are all followed up by "this shou Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@524 PS4, Line 524: // But adding a new data dir elsewhere in the list is OK. > And reword this too. Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.h@90 PS4, Line 90: or the first configured data root if it already contains : // existing metadata. > Would it be clearer if we additionally specified that this fallback behavio Hm, I didn't explicitly call it out, but I reworded it to hopefully clarify that the first data dir is only used if metadata exists there from a previous deployment. Let me know what you think. http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@93 PS4, Line 93: DEFINE_string(fs_metadata_dir, "", > There should be a mention here of the fallback behavior for existing system Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@94 PS4, Line 94: Must be equivalent to fs_wal_dir or a > I thought this wasn't true anymore? Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@95 PS4, Line 95: fs_dataA_dirs > fs_data_dirs Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@253 PS4, Line 253: // Check the first data root for metadata. > Could you LOG here what we actually used for the metadata directory, a la L Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@256 PS4, Line 256: // If there is no metadata in the first data root, use the WAL root. > Hmm, but we wouldn't want to do this fallback when creating a brand new fil Is your concern that if the FS layout already partially exists, but not completely (such that fs_manager()->Open() fails, but fs_manager()->Create() succeeds), we may end up going through CreateInitialFileSystemLayout() with the first data root storing metadata? That could happen if they fail to start up a cluster with Kudu 1.6, upgrade to Kudu 1.7, and then start up successfully. I think I'm ok with this though, although a simple update would be to also check whether the metadata directory is empty, in which case we could use the WAL dir. What do you think? http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@265 PS4, Line 265: const auto& canonicalized_metadata_root = canonicalized_metadata_fs_root_; > Why do we need this local? Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@421 PS4, Line 421: if (dd_manager_->FindUuidIndexByRoot(canonicalized_metadata_fs_root_.path, &metadata_idx) && > I'm confused; why would the DataDirManager be aware of the metadata root, s Hm, good point. It's not important and it's just noise moving forward. http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@547 PS4, Line 547: . > Nit: drop the period Debugging string, dropping it altogether. -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 16 Jan 2018 20:25:54 +0000 Gerrit-HasComments: Yes
