David Ribeiro Alves has posted comments on this change. Change subject: open FS layout in presence of disk failure ......................................................................
Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/7784/4/src/kudu/fs/block_manager_util.h File src/kudu/fs/block_manager_util.h: PS4, Line 56: // On success, either 'metadata_' is overwritten with the contents of the : // file, or, in the case of disk failure, returns OK and sets : // 'health_status_' to a non-OK value. : Status LoadFromDisk(); > Disk failure errors here would be ignored anyway. By adding this to the con I'm not sure I fully agree. For one the error message is confusing: "LOG(ERROR) << "Ignoring error with status: " << _s_prepended.ToString(); \" seems like we're ignoring the error when we really aren't. Then I'd expect that upon getting an OK here the metadata had been loaded, when it's not the case. Given there is a single call site I think it would make sense to be explicit. That is to return a non-ok status and to special case the isDiskFailure() status along with logging the error message there. Otherwise you're pushing logic from the call site onto this class which is a bit dirtier. -- To view, visit http://gerrit.cloudera.org:8080/7784 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib2a1c824526ed52a6b90ddfbc735cecc4c491118 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
