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 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 b I had been thinking of just renaming update_on_disk to something like ignore_inconsistencies (and doc that it may update the on-disk structures), but I like your suggestion more. Done. 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) { > missing_roots will only be non-empty if opts.update_on_disk is true (as per Yeah, I had added a DCHECK and comment to that effect to the equivalent change in data_dirs.cc, but not here: // Add new or remove existing data directories, if desired. if (!opts_.read_only && (!missing_roots.empty() || has_missing_instance)) { DCHECK(opts_.update_on_disk); // guaranteed by LoadInstances and // the ternary in CheckIntegrity above Anyway, we don't need the read_only check any more because read_only + update_on_disk is forbidden once again. I'll update this. 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 direc It should work, but the current code fails in the integrity checks when re-adding a previously removed data directory. So I modified DataDirManager::Open to elide all integrity checks when updating. This also allowed me to get rid of the CheckIntegrity changes which were pretty ugly to begin with. I also beefed up this test and added a couple more fs_manager-test cases. Note that in real life, although the tserver can unfail these tablets, the master may kick off rereplication for them anyway. 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: Done -- 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 <a...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Sat, 20 Jan 2018 00:12:41 +0000 Gerrit-HasComments: Yes
