Adar Dembo has posted comments on this change. Change subject: KUDU-1377: Ignore corrupted trailing records on LBM metadata ......................................................................
Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/2595/3/src/kudu/util/pb_util.cc File src/kudu/util/pb_util.cc: Line 458: // Avoid returning Status::EndOfFile to caller since it may be confusing. > Well, this was a suggestion you made on the previous rev, but I've now reve It must have been something I said in chat, because the only suggestion I made in the first rev was to disambiguate between the "safe" corruption case (hit the end of the file while processing a record) and the "unsafe" case (encountered some other kind of error, not EOF-related, while processing a record). In any case, sorry if I misled, I think this is clearer. Line 476: return Status::EndOfFile("Reached end of file"); > No, there is no partial record. It is truly the end of the file. I've added I see. I assumed that, like in many other functions, an OUT parameter is set on any failure, but here it's only set on a particular kind of failure (i.e. Status::Corruption). That's dangerous as most developers are used to reading OUT parameters in any failure case. In fact, pb_util-test.cc makes that mistake in TestMultipleMessages: is_partial_record is read from regardless of the result of ReadNextPB, which means the caller is reading garbage data if the function were to return here. -- To view, visit http://gerrit.cloudera.org:8080/2595 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0b88ba051b390b6a2587eecdd205c478f1edccb4 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-HasComments: Yes
