Alexey Serbin 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))); > This code is changing in the next patch anyway; I've addressed this feedbac SGTM 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())); : } > This is a graceful failure check while AppendSegmentUnlocked is a CHECK. If Yup, converting CHECK in AppendSegmentUnlocked() into DCHECK makes sense to me. -- 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-Comment-Date: Tue, 03 Dec 2019 21:08:18 +0000 Gerrit-HasComments: Yes
