Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14760 )
Change subject: KUDU-2993: don't require update_dirs to fix directory inconsistencies ...................................................................... Patch Set 11: (18 comments) http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/block_manager_util-test.cc File src/kudu/fs/block_manager_util-test.cc: http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/block_manager_util-test.cc@a98 PS10, Line 98: > Removed because it wasn't necessary for the test? Yeah, though on second thought I guess I'll keep it for consistency with the other usages. http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/block_manager_util.h File src/kudu/fs/block_manager_util.h: http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/block_manager_util.h@41 PS10, Line 41: // > At first blush, the addition of a UUID here is weird if our intent is to ca Right, good summary of why this is necessary. I've taken the pertinent bits and commented down by uuid_. Done http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/block_manager_util.h@55 PS10, Line 55: // > I'll probably get this answered when I review the rest of the change, but w Why should we tie the implementation of this class to protobuf? The abstraction is the "path set", not the protobuf. If protobuf had a set primitive, I think we'd use it for the 'path_set' field. Plus, there'd a "conversion" from vector to RepeatedField regardless. A set ensures there are no duplicates, which is more aligned with what we want out of the 'all_uuids' field. It also makes it easier for us to cross-reference our 'uuid_', which must exist. I'll add some of that to the description here. http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/block_manager_util.h@105 PS10, Line 105: // it can be used when creating a new instance file. However, it may be : // overwritten when loading > Why this rename? Sorry, I started down the path of refactoring, but yeah this shouldn't be in this patch. http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/block_manager_util.cc File src/kudu/fs/block_manager_util.cc: http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/block_manager_util.cc@151 PS10, Line 151: RETURN_NOT_OK_FAIL_INSTANCE_PREPEND( > Do we need to delete the created directory if we fail further down? In the Done http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/block_manager_util.cc@154 PS10, Line 154: auto cleanup_dir_on_failure = MakeScopedCleanup([&] { : if (created) { : > Don't you want to enforce the (usual) contract where if a function fails, i Done http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/block_manager_util.cc@243 PS10, Line 243: metadata_ = std::move(pb); > Why did you remove the "different user" part? Done http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/data_dirs.h@454 PS10, Line 454: bool* has_existing_instances); > Is it strictly necessary for this OUT parameter to be optional? Could we re No optionality is strictly necessary. Sure. http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/data_dirs.cc@a377 PS10, Line 377: > Do we need to enforce !read-only if one of the UPDATE_AND_ behaviors was ch Done http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/data_dirs.cc@416 PS10, Line 416: CHECK(!opts_.read_only); > Use CHECK_NE instead. Done http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/data_dirs.cc@448 PS10, Line 448: > This was a little ambiguous; does it refer to "any instance whatsoever"? Or Done http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/data_dirs.cc@550 PS10, Line 550: > update Done http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/data_dirs.cc@608 PS10, Line 608: > load Done http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/data_dirs.cc@779 PS10, Line 779: if (fs_check.ok()) { > In keeping with the existing behavior, there's no reason to go down this br Uglier branching structure, but done. http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/fs.proto File src/kudu/fs/fs.proto: http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/fs.proto@60 PS10, Line 60: message PathInstanceMetadataPB { > Won't this break old readers that try to deserialize a message with the (no Yeah sorry, this shouldn't be in this patch. Will do this in a follow-up when I refactor out some directory-management code. I don't think that use case will exist for existing block managers. http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/fs_manager.h@289 PS10, Line 289: FRIEND_TEST(fs::FsManagerTestBase, TestDuplicatePaths); : FRIEND_TEST(fs::FsManagerTestBase, TestEIOWhileRunningUpdateDirsTool); : FRIEND_TEST(fs::FsManagerTestBase, TestIsolatedMetadataDir); : FRIEND_TEST(fs::FsManagerTestBase, TestMetadataDirInWALRoot); : FRIEND_TEST(fs::FsManagerTestBase, TestMetadataDirInDataRoot); : FRIEND_TEST(fs::FsManagerTestBase, TestOpenWithDuplicateInstanceFiles); > Want to just "friend class fs::FsManagerTestBase"? I would, but I don't think that works since each test is its own class. http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/fs_manager.cc@a163 PS10, Line 163: > Don't we still want to enforce that if you picked UPDATE_AND, you can't use Done http://gerrit.cloudera.org:8080/#/c/14760/10/src/kudu/fs/fs_manager.cc@396 PS10, Line 396: instances == UpdateInstanceBehavior::UPDATE > Deduplicate this string. Done -- To view, visit http://gerrit.cloudera.org:8080/14760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic3027e7edb5c60e96ced6160fec1a380b38353a5 Gerrit-Change-Number: 14760 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong <[email protected]> Gerrit-Comment-Date: Thu, 05 Dec 2019 00:25:14 +0000 Gerrit-HasComments: Yes
