You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/11/08 00:38:34 UTC
[kudu-CR] KUDU-1677. Scanners can return out-of-order rows during a compaction
Hello David Ribeiro Alves, Anonymous Coward #80,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4983
to review the following change.
Change subject: KUDU-1677. Scanners can return out-of-order rows during a compaction
......................................................................
KUDU-1677. Scanners can return out-of-order rows during a compaction
This fixes a bug where a scanner could return out-of-order rows if
started during the "duplicating" phase of a compaction.
The issue was that DuplicatingRowSet would create a UnionIterator rather
than a MergeIterator despite the fact that the root iterator was
supposed to be in an ordered mode.
Fixing this involved passing the OrderMode down into the NewRowIterator
call. This argument is only used by the DuplicatingRowSet for now, but
may be useful for other rowset types down the road (e.g. the MemRowSet
might be able to scan faster if it could scan in insertion order instead
of in key order).
I also removed the redundant Tablet::OrderMode enum and instead just
passed the protobuf-based enum (from common.proto) all the way through,
which simplified things a little bit.
Change-Id: I6d5c373e2d56039a3b8be272cb4fea3d5d840e5b
---
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/mt-rowset_delta_compaction-test.cc
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/tablet-test-base.h
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tserver/tablet_service.cc
18 files changed, 112 insertions(+), 68 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/4983/1
--
To view, visit http://gerrit.cloudera.org:8080/4983
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6d5c373e2d56039a3b8be272cb4fea3d5d840e5b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
[kudu-CR] KUDU-1677. Scanners can return out-of-order rows during a compaction
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1677. Scanners can return out-of-order rows during a compaction
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/4983/1/src/kudu/tablet/diskrowset.h
File src/kudu/tablet/diskrowset.h:
Line 315: virtual Status NewRowIterator(const Schema *projection,
> warning: function 'kudu::tablet::DiskRowSet::NewRowIterator' has a definiti
Done
http://gerrit.cloudera.org:8080/#/c/4983/1/src/kudu/tablet/major_delta_compaction-test.cc
File src/kudu/tablet/major_delta_compaction-test.cc:
Line 145: void VerifyDataWithMvccAndExpectedState(MvccSnapshot& snap,
> warning: non-const reference parameter 'snap', make it const or use a point
Done
http://gerrit.cloudera.org:8080/#/c/4983/1/src/kudu/tablet/rowset.cc
File src/kudu/tablet/rowset.cc:
Line 79: } else {
> warning: don't use else after return [readability-else-after-return]
Done
http://gerrit.cloudera.org:8080/#/c/4983/1/src/kudu/tablet/tablet-test-base.h
File src/kudu/tablet/tablet-test-base.h:
Line 422: boost::optional<TestRowVerifier> verifier) {
> warning: the parameter 'verifier' is copied for each invocation but only us
Done
http://gerrit.cloudera.org:8080/#/c/4983/1/src/kudu/tablet/tablet-test.cc
File src/kudu/tablet/tablet-test.cc:
Line 492: bool TestSetupExpectsNulls(int32_t key_idx) {
> warning: parameter 'key_idx' is unused [misc-unused-parameters]
Done
--
To view, visit http://gerrit.cloudera.org:8080/4983
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d5c373e2d56039a3b8be272cb4fea3d5d840e5b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1677. Scanners can return out-of-order rows during a compaction
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1677. Scanners can return out-of-order rows during a compaction
......................................................................
Patch Set 3:
I presume the new test failed without the fix?
--
To view, visit http://gerrit.cloudera.org:8080/4983
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d5c373e2d56039a3b8be272cb4fea3d5d840e5b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1677. Scanners can return out-of-order rows during a compaction
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Anonymous Coward #80, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4983
to look at the new patch set (#3).
Change subject: KUDU-1677. Scanners can return out-of-order rows during a compaction
......................................................................
KUDU-1677. Scanners can return out-of-order rows during a compaction
This fixes a bug where a scanner could return out-of-order rows if
started during the "duplicating" phase of a compaction.
The issue was that DuplicatingRowSet would create a UnionIterator rather
than a MergeIterator despite the fact that the root iterator was
supposed to be in an ordered mode.
Fixing this involved passing the OrderMode down into the NewRowIterator
call. This argument is only used by the DuplicatingRowSet for now, but
may be useful for other rowset types down the road (e.g. the MemRowSet
might be able to scan faster if it could scan in insertion order instead
of in key order).
I also removed the redundant Tablet::OrderMode enum and instead just
passed the protobuf-based enum (from common.proto) all the way through,
which simplified things a little bit.
Change-Id: I6d5c373e2d56039a3b8be272cb4fea3d5d840e5b
---
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/mt-rowset_delta_compaction-test.cc
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/tablet-test-base.h
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tserver/tablet_service.cc
18 files changed, 128 insertions(+), 85 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/4983/3
--
To view, visit http://gerrit.cloudera.org:8080/4983
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6d5c373e2d56039a3b8be272cb4fea3d5d840e5b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1677. Scanners can return out-of-order rows during a compaction
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1677. Scanners can return out-of-order rows during a compaction
......................................................................
Patch Set 3:
Yea, it did.
--
To view, visit http://gerrit.cloudera.org:8080/4983
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d5c373e2d56039a3b8be272cb4fea3d5d840e5b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1677. Scanners can return out-of-order rows during a compaction
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Anonymous Coward #80, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4983
to look at the new patch set (#2).
Change subject: KUDU-1677. Scanners can return out-of-order rows during a compaction
......................................................................
KUDU-1677. Scanners can return out-of-order rows during a compaction
This fixes a bug where a scanner could return out-of-order rows if
started during the "duplicating" phase of a compaction.
The issue was that DuplicatingRowSet would create a UnionIterator rather
than a MergeIterator despite the fact that the root iterator was
supposed to be in an ordered mode.
Fixing this involved passing the OrderMode down into the NewRowIterator
call. This argument is only used by the DuplicatingRowSet for now, but
may be useful for other rowset types down the road (e.g. the MemRowSet
might be able to scan faster if it could scan in insertion order instead
of in key order).
I also removed the redundant Tablet::OrderMode enum and instead just
passed the protobuf-based enum (from common.proto) all the way through,
which simplified things a little bit.
Change-Id: I6d5c373e2d56039a3b8be272cb4fea3d5d840e5b
---
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/mt-rowset_delta_compaction-test.cc
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/tablet-test-base.h
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tserver/tablet_service.cc
18 files changed, 112 insertions(+), 68 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/4983/2
--
To view, visit http://gerrit.cloudera.org:8080/4983
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6d5c373e2d56039a3b8be272cb4fea3d5d840e5b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1677. Scanners can return out-of-order rows during a compaction
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: KUDU-1677. Scanners can return out-of-order rows during a compaction
......................................................................
KUDU-1677. Scanners can return out-of-order rows during a compaction
This fixes a bug where a scanner could return out-of-order rows if
started during the "duplicating" phase of a compaction.
The issue was that DuplicatingRowSet would create a UnionIterator rather
than a MergeIterator despite the fact that the root iterator was
supposed to be in an ordered mode.
Fixing this involved passing the OrderMode down into the NewRowIterator
call. This argument is only used by the DuplicatingRowSet for now, but
may be useful for other rowset types down the road (e.g. the MemRowSet
might be able to scan faster if it could scan in insertion order instead
of in key order).
I also removed the redundant Tablet::OrderMode enum and instead just
passed the protobuf-based enum (from common.proto) all the way through,
which simplified things a little bit.
Change-Id: I6d5c373e2d56039a3b8be272cb4fea3d5d840e5b
Reviewed-on: http://gerrit.cloudera.org:8080/4983
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/mt-rowset_delta_compaction-test.cc
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/tablet-test-base.h
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tserver/tablet_service.cc
18 files changed, 128 insertions(+), 85 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4983
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I6d5c373e2d56039a3b8be272cb4fea3d5d840e5b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1677. Scanners can return out-of-order rows during a compaction
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1677. Scanners can return out-of-order rows during a compaction
......................................................................
Patch Set 3: Code-Review+2
Oops, meant to +2.
--
To view, visit http://gerrit.cloudera.org:8080/4983
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d5c373e2d56039a3b8be272cb4fea3d5d840e5b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1677. Scanners can return out-of-order rows during a compaction
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1677. Scanners can return out-of-order rows during a compaction
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4983/3/src/kudu/tablet/tablet-test-base.h
File src/kudu/tablet/tablet-test-base.h:
Line 429: gscoped_ptr<RowwiseIterator> iter,
> warning: the parameter 'iter' is copied for each invocation but only used a
this is actually bogus, since the move ctor of gscoped_ptr has an intended "consume" side effect
--
To view, visit http://gerrit.cloudera.org:8080/4983
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d5c373e2d56039a3b8be272cb4fea3d5d840e5b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes