Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8352 )
Change subject: tool: new actions for adding and removing data directories ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/8352/2/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/8352/2/src/kudu/tools/kudu-tool-test.cc@2330 PS2, Line 2330: ASSERT_STR_CONTAINS(s.ToString(), "Can't find block"); > shouldn't the tablet fail because it sees that one of its disks is now miss We do data dir group resolution while loading the tablet's superblock, and hard failing there means the tserver will fail to start. I tried this at first (by modifying DataDirGroup::FromPB to return a bad status when we can't find a data dir) and that's what I got. Separately, remind me what's the problem with block ID reuse? I know we have a problem in tests where we lack the ability to clear the block cache, but what's wrong with reuse in production, across distinct tserver process instances? http://gerrit.cloudera.org:8080/#/c/8352/2/src/kudu/tools/tool_action_fs.cc File src/kudu/tools/tool_action_fs.cc: http://gerrit.cloudera.org:8080/#/c/8352/2/src/kudu/tools/tool_action_fs.cc@348 PS2, Line 348: Status AddDataDir(const RunnerContext& context) { > when adding a data dir, should we have some option to add it to existing ta That's a good idea, but it'd mean rewriting all the superblocks, so it's not trivial to do when you factor in the cleanup work in the event of an error. Can I punt on it for now? http://gerrit.cloudera.org:8080/#/c/8352/2/src/kudu/tools/tool_action_fs.cc@554 PS2, Line 554: Any tablets with data on the removed data directory " : "will fail the next time the server is started > Sort of had a similar thought: perhaps a mandatory unsafe "force" flag if t I'd rather not introduce interactivity to this particular action, but I'm OK with Andrew's idea to require users to pass an "optional" --force parameter. I can check for the presence of containers as an approximation for "is there live data?", and require users to use --force if there is. -- To view, visit http://gerrit.cloudera.org:8080/8352 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6ddbdd6cc6231996e7802a622a8b4691527a0643 Gerrit-Change-Number: 8352 Gerrit-PatchSet: 2 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: Mon, 23 Oct 2017 23:46:08 +0000 Gerrit-HasComments: Yes
