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/01/04 00:32:53 UTC
[kudu-CR] generic iterators: assorted cleanup
Hello Mike Percy,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/12156
to review the following change.
Change subject: generic_iterators: assorted cleanup
......................................................................
generic_iterators: assorted cleanup
This patch cleans up the MergeIterator and makes it look more like the
UnionIterator:
1. Addressed a long-standing TODO about schema verification.
2. Extracted the schema from the sub-iterators.
Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544
---
M src/kudu/common/generic_iterators-test.cc
M src/kudu/common/generic_iterators.cc
M src/kudu/common/generic_iterators.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/tablet.cc
5 files changed, 88 insertions(+), 62 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/12156/1
--
To view, visit http://gerrit.cloudera.org:8080/12156
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544
Gerrit-Change-Number: 12156
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] generic iterators: assorted cleanup
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/12156
to look at the new patch set (#4).
Change subject: generic_iterators: assorted cleanup
......................................................................
generic_iterators: assorted cleanup
This patch cleans up the MergeIterator and makes it look more like the
UnionIterator:
1. Addressed a long-standing TODO about schema verification.
2. Extracted the schema from the sub-iterators.
Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544
---
M src/kudu/common/generic_iterators-test.cc
M src/kudu/common/generic_iterators.cc
M src/kudu/common/generic_iterators.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/tablet.cc
5 files changed, 100 insertions(+), 74 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/12156/4
--
To view, visit http://gerrit.cloudera.org:8080/12156
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544
Gerrit-Change-Number: 12156
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] generic iterators: assorted cleanup
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12156 )
Change subject: generic_iterators: assorted cleanup
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12156
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544
Gerrit-Change-Number: 12156
Gerrit-PatchSet: 5
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 (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 23 Jan 2019 22:52:24 +0000
Gerrit-HasComments: No
[kudu-CR] generic iterators: assorted cleanup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12156 )
Change subject: generic_iterators: assorted cleanup
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12156/4/src/kudu/common/generic_iterators.h
File src/kudu/common/generic_iterators.h:
http://gerrit.cloudera.org:8080/#/c/12156/4/src/kudu/common/generic_iterators.h@51
PS4, Line 51: otherwise the merge algorithm's key comparisons won't work
> nit: this is a bit odd wording. Will we get some incorrect results? Or will
It's tricky. There's a CHECK that'll fire if the projection has zero key columns, but we can't enforce that it actually has _all_ of the columns that comprise the primary key, because the MergeIterator doesn't have access to the tablet's schema and thus doesn't know what they all are.
There's code in the scan setup (tablet_service.cc) that will add any missing key columns to an ORDERED scan's projection, so it shouldn't be possible to actually construct a non-conformant MergeIterator in production. But it is possible if you looked at MergeIterator in isolation.
I'll move the comment and expand on this a bit.
http://gerrit.cloudera.org:8080/#/c/12156/4/src/kudu/common/generic_iterators.cc
File src/kudu/common/generic_iterators.cc:
http://gerrit.cloudera.org:8080/#/c/12156/4/src/kudu/common/generic_iterators.cc@281
PS4, Line 281: states_.push_back(unique_ptr<MergeIterState
> nit: emplace_back and you can avoid the unique_ptr<> bit?
It's a bit moot because later on I'm converting states_ into an intrusive list which doesn't support emplace_back.
--
To view, visit http://gerrit.cloudera.org:8080/12156
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544
Gerrit-Change-Number: 12156
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 (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 14 Jan 2019 21:04:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] generic iterators: assorted cleanup
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/12156
to look at the new patch set (#5).
Change subject: generic_iterators: assorted cleanup
......................................................................
generic_iterators: assorted cleanup
This patch cleans up the MergeIterator and makes it look more like the
UnionIterator:
1. Addressed a long-standing TODO about schema verification.
2. Extracted the schema from the sub-iterators.
Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544
---
M src/kudu/common/generic_iterators-test.cc
M src/kudu/common/generic_iterators.cc
M src/kudu/common/generic_iterators.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/tablet.cc
5 files changed, 100 insertions(+), 74 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/12156/5
--
To view, visit http://gerrit.cloudera.org:8080/12156
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544
Gerrit-Change-Number: 12156
Gerrit-PatchSet: 5
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 (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] generic iterators: assorted cleanup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12156 )
Change subject: generic_iterators: assorted cleanup
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12156/3/src/kudu/common/generic_iterators.h
File src/kudu/common/generic_iterators.h:
http://gerrit.cloudera.org:8080/#/c/12156/3/src/kudu/common/generic_iterators.h@58
PS3, Line 58: // All iterators must be fully able to evaluate all predicates (i.e. calling
: // iter->Init(spec) should remove all predicates from the ScanSpec).
> is this true? i thought the fact that we use InitAndMaybeWrap means that we
Hmm, yes, that appears to be the case. I copied this comment from UnionIterator thinking it that if it's true there it must be true for both since both use the same "maybe wrapping" approach.
I'll remove the comment from both.
http://gerrit.cloudera.org:8080/#/c/12156/3/src/kudu/common/generic_iterators.cc
File src/kudu/common/generic_iterators.cc:
http://gerrit.cloudera.org:8080/#/c/12156/3/src/kudu/common/generic_iterators.cc@247
PS3, Line 247: if (!s->schema().Equals(*schema_)) {
> I'd be on board with making this a DCHECK.
It's on in all builds for UnionIterator so I assume we were comfortable with the perf hit. But I don't feel strongly, and since Mike prefers making it DEBUG-only, I'll make that change.
http://gerrit.cloudera.org:8080/#/c/12156/3/src/kudu/common/generic_iterators.cc@249
PS3, Line 249: string("Schemas do not match: ") + schema_->ToString()
: + " vs " + s->schema().ToString());
> this is somewhat non-idiomatic (why not use Substitute?)
Copied code from UnionIterator. Will fix in both.
http://gerrit.cloudera.org:8080/#/c/12156/3/src/kudu/common/generic_iterators.cc@472
PS3, Line 472: s.append("Union(");
> I'd be inclined to do this in a follow up because I am modifying this same
I don't mind doing this now; Mike's concern is valid but I don't think he's touching UnionIterator.
--
To view, visit http://gerrit.cloudera.org:8080/12156
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544
Gerrit-Change-Number: 12156
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 (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 11 Jan 2019 23:52:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] generic iterators: assorted cleanup
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12156 )
Change subject: generic_iterators: assorted cleanup
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12156/4/src/kudu/common/generic_iterators.h
File src/kudu/common/generic_iterators.h:
http://gerrit.cloudera.org:8080/#/c/12156/4/src/kudu/common/generic_iterators.h@51
PS4, Line 51: otherwise the merge algorithm's key comparisons won't work
nit: this is a bit odd wording. Will we get some incorrect results? Or will it fail a CHECK? or what? It seems like it should fail a CHECK and this doc should explicitly say that it REQUIRES that the projection include key columns (and maybe move that comment to the constructor which takes the list of iters)
http://gerrit.cloudera.org:8080/#/c/12156/4/src/kudu/common/generic_iterators.cc
File src/kudu/common/generic_iterators.cc:
http://gerrit.cloudera.org:8080/#/c/12156/4/src/kudu/common/generic_iterators.cc@281
PS4, Line 281: states_.push_back(unique_ptr<MergeIterState
nit: emplace_back and you can avoid the unique_ptr<> bit?
--
To view, visit http://gerrit.cloudera.org:8080/12156
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544
Gerrit-Change-Number: 12156
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 (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 12 Jan 2019 01:05:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] generic iterators: assorted cleanup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12156 )
Change subject: generic_iterators: assorted cleanup
......................................................................
generic_iterators: assorted cleanup
This patch cleans up the MergeIterator and makes it look more like the
UnionIterator:
1. Addressed a long-standing TODO about schema verification.
2. Extracted the schema from the sub-iterators.
Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544
Reviewed-on: http://gerrit.cloudera.org:8080/12156
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/common/generic_iterators-test.cc
M src/kudu/common/generic_iterators.cc
M src/kudu/common/generic_iterators.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/tablet.cc
5 files changed, 100 insertions(+), 74 deletions(-)
Approvals:
Kudu Jenkins: Verified
Mike Percy: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/12156
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544
Gerrit-Change-Number: 12156
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 (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] generic iterators: assorted cleanup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12156
to look at the new patch set (#3).
Change subject: generic_iterators: assorted cleanup
......................................................................
generic_iterators: assorted cleanup
This patch cleans up the MergeIterator and makes it look more like the
UnionIterator:
1. Addressed a long-standing TODO about schema verification.
2. Extracted the schema from the sub-iterators.
Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544
---
M src/kudu/common/generic_iterators-test.cc
M src/kudu/common/generic_iterators.cc
M src/kudu/common/generic_iterators.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/tablet.cc
5 files changed, 95 insertions(+), 64 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/12156/3
--
To view, visit http://gerrit.cloudera.org:8080/12156
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544
Gerrit-Change-Number: 12156
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] generic iterators: assorted cleanup
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12156 )
Change subject: generic_iterators: assorted cleanup
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12156/3/src/kudu/common/generic_iterators.cc
File src/kudu/common/generic_iterators.cc:
http://gerrit.cloudera.org:8080/#/c/12156/3/src/kudu/common/generic_iterators.cc@247
PS3, Line 247: if (!s->schema().Equals(*schema_)) {
> is this going to be expensive in release builds? should we consider only do
I'd be on board with making this a DCHECK.
http://gerrit.cloudera.org:8080/#/c/12156/3/src/kudu/common/generic_iterators.cc@472
PS3, Line 472: s.append("Union(");
> while we're here, perhaps we can clean up this function to do:
I'd be inclined to do this in a follow up because I am modifying this same file and conflicting all over the place with this patch series so would like to maintain velocity and granularity for these changes.
--
To view, visit http://gerrit.cloudera.org:8080/12156
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544
Gerrit-Change-Number: 12156
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 09 Jan 2019 22:18:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] generic iterators: assorted cleanup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12156
to look at the new patch set (#2).
Change subject: generic_iterators: assorted cleanup
......................................................................
generic_iterators: assorted cleanup
This patch cleans up the MergeIterator and makes it look more like the
UnionIterator:
1. Addressed a long-standing TODO about schema verification.
2. Extracted the schema from the sub-iterators.
Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544
---
M src/kudu/common/generic_iterators-test.cc
M src/kudu/common/generic_iterators.cc
M src/kudu/common/generic_iterators.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/tablet.cc
5 files changed, 93 insertions(+), 62 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/12156/2
--
To view, visit http://gerrit.cloudera.org:8080/12156
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544
Gerrit-Change-Number: 12156
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] generic iterators: assorted cleanup
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12156 )
Change subject: generic_iterators: assorted cleanup
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12156/3/src/kudu/common/generic_iterators.h
File src/kudu/common/generic_iterators.h:
http://gerrit.cloudera.org:8080/#/c/12156/3/src/kudu/common/generic_iterators.h@58
PS3, Line 58: // All iterators must be fully able to evaluate all predicates (i.e. calling
: // iter->Init(spec) should remove all predicates from the ScanSpec).
is this true? i thought the fact that we use InitAndMaybeWrap means that we handle the necessary wrapping internal to this class if the caller passes in one that doesn't handle predicates
http://gerrit.cloudera.org:8080/#/c/12156/3/src/kudu/common/generic_iterators.cc
File src/kudu/common/generic_iterators.cc:
http://gerrit.cloudera.org:8080/#/c/12156/3/src/kudu/common/generic_iterators.cc@247
PS3, Line 247: if (!s->schema().Equals(*schema_)) {
is this going to be expensive in release builds? should we consider only doing it in debug? or should we wait until proven to be a hotspot before optimizing it out?
http://gerrit.cloudera.org:8080/#/c/12156/3/src/kudu/common/generic_iterators.cc@249
PS3, Line 249: string("Schemas do not match: ") + schema_->ToString()
: + " vs " + s->schema().ToString());
this is somewhat non-idiomatic (why not use Substitute?)
http://gerrit.cloudera.org:8080/#/c/12156/3/src/kudu/common/generic_iterators.cc@472
PS3, Line 472: s.append("Union(");
while we're here, perhaps we can clean up this function to do:
s += JoinMapped(iters_, [](const shared_ptr<RowwiseIterator>& it) { return it->ToString(); });
(too bad we don't have C++14 for lambda auto arguments)
--
To view, visit http://gerrit.cloudera.org:8080/12156
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544
Gerrit-Change-Number: 12156
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 09 Jan 2019 18:15:48 +0000
Gerrit-HasComments: Yes