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 11: (6 comments) http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/fs/fs_manager-test.cc File src/kudu/fs/fs_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/fs/fs_manager-test.cc@265 PS11, Line 265: // We should be able to explicate that the metadata is in the WAL root. Nit: is 'explicate' the right choice here? I substitute it with 'analyze' or 'explain' and the sentence doesn't make sense. http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/fs/fs_manager.h@268 PS11, Line 268: friend class tserver::MiniTabletServerTest_TestFsLayoutEndToEnd_Test; Isn't this exactly the same as FRIEND_TEST(MiniTabletServerTest...) above? Actually, maybe the above has no effect because you should be doing FRIEND_TEST(tserver::MiniTabletServerTest, ...)? http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/kudu-tool-test.cc@2214 PS11, Line 2214: env_->CreateDir(kTestDir); ASSERT_OK http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/kudu-tool-test.cc@2229 PS11, Line 2229: RunTool(Substitute("fs dump uuid --fs_wal_dir=$0", opts.wal_root), nullptr, &stderr, {}, {}); ASSERT on the expected status? http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/tool_action_fs.cc File src/kudu/tools/tool_action_fs.cc: http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/tool_action_fs.cc@764 PS11, Line 764: .AddOptionalParameter("fs_metadata_dir") Since you're adding these, mind reordering the optional parameters to be alphabetically sorted? No backwards compatibility concerns; it'll just show them differently in the help. Same for the other files. http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tserver/mini_tablet_server-test.cc File src/kudu/tserver/mini_tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tserver/mini_tablet_server-test.cc@60 PS11, Line 60: younger Nit: older -- 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: 11 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-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 17 Jan 2018 22:24:44 +0000 Gerrit-HasComments: Yes
