David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 )
Change subject: KUDU-1489: allow configuration of metadata dir ...................................................................... Patch Set 6: (7 comments) first pass. mostly looked at the test. it'd be nice to have a more integration-y test that tests at least the most worrysome cases (downgrade/upgrade most likely scenarios) http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/block_manager_util.cc File src/kudu/fs/block_manager_util.cc: http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/block_manager_util.cc@176 PS6, Line 176: LoadInstnaces typo http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc File src/kudu/fs/fs_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@248 PS6, Line 248: "wal" could we be more specific? maybe look for full suffix? http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@255 PS6, Line 255: LOG(INFO) << s.ToString(); leftover from testing? if you want to print the status on failure you can do so on the assert below http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@258 PS6, Line 258: env_->DeleteRecursively(GetTestPath("wal")); : env_->DeleteRecursively(GetTestPath("data")); check the status http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@263 PS6, Line 263: opts.metadata_root = GetTestPath("data"); : ReinitFsManagerWithOpts(opts); : ASSERT_OK(fs_manager()->CreateInitialFileSystemLayout()); : ASSERT_OK(fs_manager()->Open()); : ASSERT_STR_CONTAINS(fs_manager()->GetTabletMetadataDir(), "data"); in the case before this one you test with a metadata root path that does not exist, before you delete the data/wal could you test that if the metadata root is set to the old value (like in this case) it fails too? http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@269 PS6, Line 269: // Opening the FsManager with an empty fs_metadata_dir flag should account : // for the old default and use the first data directory for metadata. wait, empty string is different from "unset"? why? http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@276 PS6, Line 276: env_->DeleteRecursively(GetTestPath("wal")); : env_->DeleteRecursively(GetTestPath("data")); same -- 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:07:00 +0000 Gerrit-HasComments: Yes
