Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8376 )
Change subject: fs: defer failure from metadata load to bootstrap when data dir is missing ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/8376/2/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: http://gerrit.cloudera.org:8080/#/c/8376/2/src/kudu/fs/data_dirs.h@81 PS2, Line 81: Status LoadFromPB(const UuidIndexByUuidMap& uuid_idx_by_uuid, > worth short docs explaining the cases of bad-status here. Done http://gerrit.cloudera.org:8080/#/c/8376/2/src/kudu/fs/data_dirs.h@84 PS2, Line 84: Status CopyToPB(const UuidByUuidIndexMap& uuid_by_uuid_idx, > and here Done http://gerrit.cloudera.org:8080/#/c/8376/2/src/kudu/fs/data_dirs.h@276 PS2, Line 276: data dir : // is missing. > only looked at the headers so far (not the impl) but this left me a little Given my response to your other comment (in the impl), is this still relevant? http://gerrit.cloudera.org:8080/#/c/8376/2/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: http://gerrit.cloudera.org:8080/#/c/8376/2/src/kudu/fs/data_dirs.cc@318 PS2, Line 318: if (!FindCopy(uuid_by_uuid_idx, uuid_idx, &uuid)) { : return Status::NotFound(Substitute( : "could not find data dir with uuid index $0", uuid_idx)); > would this not be a programmer error? how would you end up with a uuid_idx It is, but I thought the symmetry with LoadFromPB was worthwhile because it makes the class' behavior more predictable. http://gerrit.cloudera.org:8080/#/c/8376/2/src/kudu/fs/data_dirs.cc@831 PS2, Line 831: RETURN_NOT_OK(group->CopyToPB(uuid_by_idx_, pb)); > I guess my confusion in the header was wrong, but per my comments above, I Indeed, but since I want CopyToPB to be symmetric with LoadFromPB, a RETURN_NOT_OK here seemed more appropriate. http://gerrit.cloudera.org:8080/#/c/8376/2/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: http://gerrit.cloudera.org:8080/#/c/8376/2/src/kudu/tablet/tablet_metadata.cc@440 PS2, Line 440: DeleteOrphanedBlocks(orphaned_blocks); > pondering this a bit more, I wonder whether we need to actually start stori This was a legitimate issue that Andrew has since fixed in commit 47b81c452194e75da7fd966f07766de4bdcdeab0. -- To view, visit http://gerrit.cloudera.org:8080/8376 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1e8d5697c2bb08287cce11fbdab6fb8d6e37d1ad Gerrit-Change-Number: 8376 Gerrit-PatchSet: 2 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: Tue, 09 Jan 2018 20:02:40 +0000 Gerrit-HasComments: Yes
