Adar Dembo 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_metadata_dir somehow more dangerous? I guess what I'm asking is: why call this out explicitly if it works the same way as those other flags? http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG@29 PS4, Line 29: cases Nit: case 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 healthy instances? Seems like a weird dependency since this is a "util" function that ostensibly could be called from anywhere. 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? 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? 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 the same? 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. 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 should fail", but this no longer fails. Reword? 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. 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 behavior only takes effect when opening an existing filesystem? That is, when creating a new filesystem, it's just "verbatim, or WAL dir if empty"? 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 systems. 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? http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@95 PS4, Line 95: fs_dataA_dirs fs_data_dirs 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 L243-244? 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 filesystem. Does it actually matter? Is it possible for CreateFileSystemLayout to succeed if meta_dir_in_data_root already exists? 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? 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, since it's no longer guaranteed to be colocated with the data directory? Oh, this is just for the case where they are colocated? What's the value in this check, then, since it won't fire for new deployments? http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@547 PS4, Line 547: . Nit: drop the period -- 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: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 16 Jan 2018 19:09:44 +0000 Gerrit-HasComments: Yes
