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 2018/06/22 22:03:57 UTC
[kudu-CR] tablet: encapsulate common iterator options
Hello Mike Percy, Grant Henke, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/10802
to review the following change.
Change subject: tablet: encapsulate common iterator options
......................................................................
tablet: encapsulate common iterator options
I intend to introduce an additional option or two as part of the "diff scan"
API. To that end, this patch knocks the plumbing out of the way by moving
all of the common iterator options into a new struct.
Unfortunately, I can't use it in Tablet::NewRowIterator because while nearly
every iterator expects a pointer to the projection, Tablet::Iterator stores
a copy of the projection itself.
Change-Id: I7232d163436e69999bba75ed66756d2a86c5a959
---
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_compaction-test.cc
M src/kudu/tablet/delta_iterator_merger.cc
M src/kudu/tablet/delta_iterator_merger.h
M src/kudu/tablet/delta_store.h
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/deltamemstore.cc
M src/kudu/tablet/deltamemstore.h
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/memrowset-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-util.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
27 files changed, 259 insertions(+), 241 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/10802/1
--
To view, visit http://gerrit.cloudera.org:8080/10802
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7232d163436e69999bba75ed66756d2a86c5a959
Gerrit-Change-Number: 10802
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tablet: encapsulate common iterator options
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10802 )
Change subject: tablet: encapsulate common iterator options
......................................................................
Patch Set 6: Verified+1
Overriding Jenkins, unrelated test failure.
--
To view, visit http://gerrit.cloudera.org:8080/10802
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7232d163436e69999bba75ed66756d2a86c5a959
Gerrit-Change-Number: 10802
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 18 Jul 2018 02:26:18 +0000
Gerrit-HasComments: No
[kudu-CR] tablet: encapsulate common iterator options
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10802 )
Change subject: tablet: encapsulate common iterator options
......................................................................
Patch Set 4: Verified+1
Overriding Jenkins, filed KUDU-2496 for the flake.
--
To view, visit http://gerrit.cloudera.org:8080/10802
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7232d163436e69999bba75ed66756d2a86c5a959
Gerrit-Change-Number: 10802
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 11 Jul 2018 00:05:58 +0000
Gerrit-HasComments: No
[kudu-CR] tablet: encapsulate common iterator options
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10802 )
Change subject: tablet: encapsulate common iterator options
......................................................................
Patch Set 6: Code-Review+2
LGTM if Todd is onboard
--
To view, visit http://gerrit.cloudera.org:8080/10802
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7232d163436e69999bba75ed66756d2a86c5a959
Gerrit-Change-Number: 10802
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Jul 2018 17:23:03 +0000
Gerrit-HasComments: No
[kudu-CR] tablet: encapsulate common iterator options
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10802 )
Change subject: tablet: encapsulate common iterator options
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10802/3/src/kudu/tablet/deltafile.cc
File src/kudu/tablet/deltafile.cc:
http://gerrit.cloudera.org:8080/#/c/10802/3/src/kudu/tablet/deltafile.cc@a369
PS3, Line 369:
> hmm, what happened to empty_schema? seems now we are letting it be an unini
My bad. That's because in an earlier version of this patch, opts.schema had type Schema, not "const Schema*".
http://gerrit.cloudera.org:8080/#/c/10802/3/src/kudu/tablet/deltafile.cc@691
PS3, Line 691: tlipcon
> nit: i usually use 'todd' since it's my apache id
Done
http://gerrit.cloudera.org:8080/#/c/10802/3/src/kudu/tablet/rowset.h
File src/kudu/tablet/rowset.h:
http://gerrit.cloudera.org:8080/#/c/10802/3/src/kudu/tablet/rowset.h@67
PS3, Line 67: OrderMode order;
> seems there are lots of places where this struct is instantiated without ac
Sure, I'll add a default constructor that does something useful.
--
To view, visit http://gerrit.cloudera.org:8080/10802
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7232d163436e69999bba75ed66756d2a86c5a959
Gerrit-Change-Number: 10802
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 10 Jul 2018 22:14:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] tablet: encapsulate common iterator options
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/10802 )
Change subject: tablet: encapsulate common iterator options
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/10802
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I7232d163436e69999bba75ed66756d2a86c5a959
Gerrit-Change-Number: 10802
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tablet: encapsulate common iterator options
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10802
to look at the new patch set (#4).
Change subject: tablet: encapsulate common iterator options
......................................................................
tablet: encapsulate common iterator options
I intend to introduce an additional option or two as part of the "diff scan"
API. To that end, this patch knocks the plumbing out of the way by moving
all of the common iterator options into a new struct.
Unfortunately, I can't use it in Tablet::NewRowIterator because while nearly
every iterator expects a pointer to the projection, Tablet::Iterator stores
a copy of the projection itself.
Change-Id: I7232d163436e69999bba75ed66756d2a86c5a959
---
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_compaction-test.cc
M src/kudu/tablet/delta_iterator_merger.cc
M src/kudu/tablet/delta_iterator_merger.h
M src/kudu/tablet/delta_store.h
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/deltamemstore.cc
M src/kudu/tablet/deltamemstore.h
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/memrowset-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-util.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
27 files changed, 272 insertions(+), 246 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/10802/4
--
To view, visit http://gerrit.cloudera.org:8080/10802
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7232d163436e69999bba75ed66756d2a86c5a959
Gerrit-Change-Number: 10802
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tablet: encapsulate common iterator options
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10802 )
Change subject: tablet: encapsulate common iterator options
......................................................................
tablet: encapsulate common iterator options
I intend to introduce an additional option or two as part of the "diff scan"
API. To that end, this patch knocks the plumbing out of the way by moving
all of the common iterator options into a new struct.
Unfortunately, I can't use it in Tablet::NewRowIterator because while nearly
every iterator expects a pointer to the projection, Tablet::Iterator stores
a copy of the projection itself.
Change-Id: I7232d163436e69999bba75ed66756d2a86c5a959
Reviewed-on: http://gerrit.cloudera.org:8080/10802
Tested-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_compaction-test.cc
M src/kudu/tablet/delta_iterator_merger.cc
M src/kudu/tablet/delta_iterator_merger.h
M src/kudu/tablet/delta_store.h
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/deltamemstore.cc
M src/kudu/tablet/deltamemstore.h
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/memrowset-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-util.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
27 files changed, 272 insertions(+), 246 deletions(-)
Approvals:
Adar Dembo: Verified
Mike Percy: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/10802
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I7232d163436e69999bba75ed66756d2a86c5a959
Gerrit-Change-Number: 10802
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tablet: encapsulate common iterator options
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/10802 )
Change subject: tablet: encapsulate common iterator options
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/10802
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I7232d163436e69999bba75ed66756d2a86c5a959
Gerrit-Change-Number: 10802
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tablet: encapsulate common iterator options
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10802 )
Change subject: tablet: encapsulate common iterator options
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10802/3/src/kudu/tablet/deltafile.cc
File src/kudu/tablet/deltafile.cc:
http://gerrit.cloudera.org:8080/#/c/10802/3/src/kudu/tablet/deltafile.cc@a369
PS3, Line 369:
hmm, what happened to empty_schema? seems now we are letting it be an uninitialized pointer which seems scary
http://gerrit.cloudera.org:8080/#/c/10802/3/src/kudu/tablet/deltafile.cc@691
PS3, Line 691: tlipcon
nit: i usually use 'todd' since it's my apache id
http://gerrit.cloudera.org:8080/#/c/10802/3/src/kudu/tablet/rowset.h
File src/kudu/tablet/rowset.h:
http://gerrit.cloudera.org:8080/#/c/10802/3/src/kudu/tablet/rowset.h@67
PS3, Line 67: OrderMode order;
seems there are lots of places where this struct is instantiated without actually setting all of the options. Should we have defaults for these fields?
--
To view, visit http://gerrit.cloudera.org:8080/10802
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7232d163436e69999bba75ed66756d2a86c5a959
Gerrit-Change-Number: 10802
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 07 Jul 2018 00:14:39 +0000
Gerrit-HasComments: Yes