You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2019/12/03 01:20:11 UTC
[kudu-CR] log: some cleanup and modernization
Hello Andrew Wong,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/14818
to review the following change.
Change subject: log: some cleanup and modernization
......................................................................
log: some cleanup and modernization
- Use pass by move when replacing LogReader segments.
- Remove WritableLogSegment::writable_file() private accessor. Internal code
can just as easily use writable_file_ directly.
- Make some RETURN_NOT_OK_PREPEND messages actually useful.
- Reduce unnecessary usage of more complex LogReader::Open variant.
Change-Id: I3106ede5243d05b2a43f8d43f581316b6ee7ada5
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_reader.h
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
M src/kudu/integration-tests/timestamp_advancement-itest.cc
7 files changed, 37 insertions(+), 39 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/14818/1
--
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: newchange
Gerrit-Change-Id: I3106ede5243d05b2a43f8d43f581316b6ee7ada5
Gerrit-Change-Number: 14818
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
[kudu-CR] log: some cleanup and modernization
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14818
to look at the new patch set (#2).
Change subject: log: some cleanup and modernization
......................................................................
log: some cleanup and modernization
- Use pass by move when replacing LogReader segments.
- Remove WritableLogSegment::writable_file() private accessor. Internal code
can just as easily use writable_file_ directly.
- Make some RETURN_NOT_OK_PREPEND messages actually useful.
- Reduce unnecessary usage of more complex LogReader::Open variant.
Change-Id: I3106ede5243d05b2a43f8d43f581316b6ee7ada5
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_reader.h
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
M src/kudu/integration-tests/timestamp_advancement-itest.cc
7 files changed, 37 insertions(+), 40 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/14818/2
--
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: newpatchset
Gerrit-Change-Id: I3106ede5243d05b2a43f8d43f581316b6ee7ada5
Gerrit-Change-Number: 14818
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] log: some cleanup and modernization
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 03 Dec 2019 04:45:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] log: some cleanup and modernization
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14818 )
Change subject: log: some cleanup and modernization
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
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@a644
PS2, Line 644:
Nice...
--
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 <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 03 Dec 2019 03:37:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] log: some cleanup and modernization
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14818 )
Change subject: log: some cleanup and modernization
......................................................................
log: some cleanup and modernization
- Use pass by move when replacing LogReader segments.
- Remove WritableLogSegment::writable_file() private accessor. Internal code
can just as easily use writable_file_ directly.
- Make some RETURN_NOT_OK_PREPEND messages actually useful.
- Reduce unnecessary usage of more complex LogReader::Open variant.
Change-Id: I3106ede5243d05b2a43f8d43f581316b6ee7ada5
Reviewed-on: http://gerrit.cloudera.org:8080/14818
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_reader.h
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
M src/kudu/integration-tests/timestamp_advancement-itest.cc
7 files changed, 37 insertions(+), 40 deletions(-)
Approvals:
Andrew Wong: Looks good to me, approved
Alexey Serbin: Looks good to me, but someone else must approve
Adar Dembo: Verified
--
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: merged
Gerrit-Change-Id: I3106ede5243d05b2a43f8d43f581316b6ee7ada5
Gerrit-Change-Number: 14818
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
[kudu-CR] log: some cleanup and modernization
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 03 Dec 2019 20:24:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] log: some cleanup and modernization
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Dec 2019 21:08:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] log: some cleanup and modernization
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14818 )
Change subject: log: some cleanup and modernization
......................................................................
Patch Set 2: Verified+1
Overriding Jenkins, failure was in a known flake.
--
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 <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 03 Dec 2019 20:42:36 +0000
Gerrit-HasComments: No
[kudu-CR] log: some cleanup and modernization
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/14818 )
Change subject: log: some cleanup and modernization
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
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: deleteReviewer
Gerrit-Change-Id: I3106ede5243d05b2a43f8d43f581316b6ee7ada5
Gerrit-Change-Number: 14818
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>