Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14818 )
Change subject: log: some cleanup and modernization ...................................................................... Patch Set 2: Code-Review+1 (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 not going to be used below 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 every entry being appended. Does it make sense to remove this extra verification? If not, then maybe it's worth moving this extra check out of the lock scope and perform the verification for the whole sequence of segments in 'read_segments'? -- 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: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 03 Dec 2019 04:45:52 +0000 Gerrit-HasComments: Yes
