Adar Dembo 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 2: (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" Changed to: // If 'update_on_disk' and 'read_only' are both true, the directory manager // allows the provided directories to deviate from their on-disk data // structures without updating the latter to match the former. 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 Done 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 CheckIn Hmm, I originally included this so that it isn't confusing that L659-660 omits a check for opts_.update_on_disk. I'll change it as per your first suggestion. 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. > After another look, I think this is ok. OK, I took another stab at this comment. Let me know what you think. 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 Done 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? We could, but is it necessary? We wouldn't expect tablets to just disappear, right? We don't check them after adding a data directory above. 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 Yeah, it's not intuitive that WaitStarted will return the first bootstrap failure. I'll note that. 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 I'm not sure; as you can see, List uses 'tablets' instead of 'local replicas'. -- 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: 2 Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Thu, 11 Jan 2018 00:13:40 +0000 Gerrit-HasComments: Yes
