Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8978 )
Change subject: KUDU-2202: support for removing data directories (take two) ...................................................................... Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/fs/data_dirs.h@220 PS1, Line 220: the DataDirManager is : // opened as per the new contents of of the provided directories but nit: extra "of" Also it wasn't immediately clear what "new contents of the provided directories" was referring to. Maybe consider rewording "...the DataDirManager is opened to reflect the provided directories but..." or something? (in fs_manager.cc too) http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/fs/data_dirs.h@223 PS1, Line 223: bool update_on_disk; nit: should still note the default http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/fs/data_dirs.cc@657 PS1, Line 657: Note: If 'has_missing_instance' is true, opts_.update_on_disk is : // guaranteed to be true. nit: Maybe note that this is due to the ternary parameterization of CheckIntegrity? Or note why this is important, e.g. "...so we can go ahead an update the instance files". http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/fs/data_dirs.cc@805 PS1, Line 805: // This uuid's directory is missing outright, which can happen when : // opts_.read_only and opts_.update_on_disk are both true. So this can only happen when we open our "staging" FsManager and we're missing a directory? I need to ponder this a bit more, not sure I've fully grokked this snippet. http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/fs/fs_manager.h@102 PS1, Line 102: bool update_on_disk; nit: note the default http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/tools/kudu-tool-test.cc@2232 PS1, Line 2232: ASSERT_OK(mts->WaitStarted()); Could we check that we still have all the tablets we expected? http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/tools/kudu-tool-test.cc@2240 PS1, Line 2240: // Reconfigure the tserver to drop the data directory and restart it. nit: mind adding to the comment something along the lines of, "waiting for the tablets to bootstrap should fail"? WaitStarted() and Start() are so similar, I was a surprised for a second that WaitStarted() would fail until I realized that we're waiting for the bootstraps and not for the FsManager to start up. http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/tools/tool_action_fs.cc File src/kudu/tools/tool_action_fs.cc: http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/tools/tool_action_fs.cc@854 PS1, Line 854: "Updates the set of data directories in an existing Kudu filesystem") : .ExtraDescription("If a data directory is in use by a tablet and is " : "removed, the operation will fail unless --force is also used") : .AddOptionalParameter("fs_wal_dir") : .AddOptionalParameter("fs_data_dirs") : .AddOptionalParameter("force", boost::none, string("If true, permits " : "the removal of a data directory that is configured for use by " : "existing tablets. Those tablets will fail the next time the server " : "is started")) micro-nit: I know in docs we favor being explicit, that local replicas are called replicas and tablets are a logical concept. Does the same apply for tooling? -- To view, visit http://gerrit.cloudera.org:8080/8978 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacb650115bcdf055542ef2777641a3201fc8e30b Gerrit-Change-Number: 8978 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 10 Jan 2018 03:22:04 +0000 Gerrit-HasComments: Yes
