Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14818 )
Change subject: log: some cleanup and modernization ...................................................................... Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/14818/2/src/kudu/consensus/log.cc File src/kudu/consensus/log.cc: http://gerrit.cloudera.org:8080/#/c/14818/2/src/kudu/consensus/log.cc@710 PS2, Line 710: scoped_refptr<ReadableLogSegment> readable_segment( : new ReadableLogSegment(new_segment_path, : shared_ptr<RandomAccessFile>(readable_file.release()))); : RETURN_NOT_OK(readable_segment->Init(header, new_segment->first_entry_offset())); : RETURN_NOT_OK(reader_add_segment_(std::move(readable_segment))); > nit: maybe, move this under its own scope so it's clear readable_segment is This code is changing in the next patch anyway; I've addressed this feedback there instead. http://gerrit.cloudera.org:8080/#/c/14818/2/src/kudu/consensus/log_reader.cc File src/kudu/consensus/log_reader.cc: http://gerrit.cloudera.org:8080/#/c/14818/2/src/kudu/consensus/log_reader.cc@185 PS2, Line 185: if (previous_seg_seqno != -1 && entry->header().sequence_number() != previous_seg_seqno + 1) { : return Status::Corruption(Substitute("Segment sequence numbers are not consecutive. " : "Previous segment: seqno $0, path $1; Current segment: seqno $2, path $3", : previous_seg_seqno, previous_seg_path, : entry->header().sequence_number(), entry->path())); : } > AppendSegmentUnlocked() already performs a stronger consistency check on ev This is a graceful failure check while AppendSegmentUnlocked is a CHECK. If anything, this is more useful: if someone manipulates the on-disk WAL segments and messes up the sequence numbers, a failure is better than a crash. Viewed that way, the check in AppendSegmentUnlocked is more about "defense in depth", and could even be converted into DCHECK. As for the lock scope, I'm not really concerned about it because the context is an Init() function, which runs before anyone else has access to the LogReader. -- To view, visit http://gerrit.cloudera.org:8080/14818 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3106ede5243d05b2a43f8d43f581316b6ee7ada5 Gerrit-Change-Number: 14818 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 03 Dec 2019 20:24:45 +0000 Gerrit-HasComments: Yes
