Todd Lipcon 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 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/8978/7/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: http://gerrit.cloudera.org:8080/#/c/8978/7/src/kudu/fs/fs_manager.h@104 PS7, Line 104: // If 'update_on_disk' and 'read_only' are both true, the FsManager allows : // 'data_roots' to deviate from their on-disk data structures without : // updating the latter to match the former. I'm not really a fan of these semantics -- it seems somewhat confusing to both ask to "update" and also to "not write". I prefer read_only to just enforce a sort of "fail any write ops" rather than in some cases also meaning "skip the write op." Would it be feasible to change this to some kind of enum like: enum ConsistencyCheckBehavior { // If data_roots doesn't match the on-disk path sets, fail. ENFORCE_CONSISTENCY, // If data_roots doesn't match the on-disk path sets, update the // on-disk data to match. Requires 'read_only' to be false. UPDATE_ON_DISK, // If data_roots doesn't match the on-disk path sets, // continue without updating the on-disk data. IGNORE_INCONSISTENCY }; http://gerrit.cloudera.org:8080/#/c/8978/7/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/8978/7/src/kudu/fs/fs_manager.cc@347 PS7, Line 347: if (!missing_roots.empty() && !opts_.read_only) { huh, is this an existing bug? if you use an fs_manager tool and pass an extra dir, it gets created? http://gerrit.cloudera.org:8080/#/c/8978/7/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/8978/7/src/kudu/tools/kudu-tool-test.cc@2251 PS7, Line 2251: // second table should have all failed. can we add one more step in this test which re-adds the force-removed directory? Would that properly re-enable the missing tablets? Seems like a useful "oops!" recovery option for an admin who runs the wrong command. http://gerrit.cloudera.org:8080/#/c/8978/7/src/kudu/tools/tool_action_fs.cc File src/kudu/tools/tool_action_fs.cc: http://gerrit.cloudera.org:8080/#/c/8978/7/src/kudu/tools/tool_action_fs.cc@330 PS7, Line 330: // If the user specifies --force, we assume they know what they're doing and : // skip this check. would it be possible to do the check in a new function, and something like: Status check_result = CheckTabletsWontFailWithUpdate(...); if (FLAGS_force) { WARN_NOT_OK(check_result, "blah blah continuing because you said force"); } else { RETURN_NOT_OK_PREPEND(check_result, "blah blah"); } This way if someone uses --force and then pastes us the logs of what they did, we'll at least see what happened -- 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: 7 Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Thu, 18 Jan 2018 19:35:47 +0000 Gerrit-HasComments: Yes
