You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2019/03/05 02:25:05 UTC
[kudu-CR] tserver: remove DEPRECATED range predicates
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12661
Change subject: tserver: remove DEPRECATED_range_predicates
......................................................................
tserver: remove DEPRECATED_range_predicates
This field hasn't been touched in 3 years (Kudu 0.8.0), and would
otherwise be another piece of a scan to consider for authorization.
Before moving ahead with authorization, it seems like as good a time as
any to remove it.
I rejiggered a couple of tests that were still using it to use the
preferred column_predicates field.
Change-Id: I9e338974e81321660652dd3b866efc0c9a865566
---
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
2 files changed, 18 insertions(+), 59 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/12661/1
--
To view, visit http://gerrit.cloudera.org:8080/12661
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9e338974e81321660652dd3b866efc0c9a865566
Gerrit-Change-Number: 12661
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] tserver: remove DEPRECATED range predicates
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12661
to look at the new patch set (#2).
Change subject: tserver: remove DEPRECATED_range_predicates
......................................................................
tserver: remove DEPRECATED_range_predicates
This field hasn't been touched in 3 years (Kudu 0.8.0), and would
otherwise be another piece of a scan to consider for authorization.
Before moving ahead with authorization, it seems like as good a time as
any to remove it. Using this field will now result in an error being
returned to clients.
I rejiggered a couple of tests that were still using it to use the
preferred column_predicates field, and added a test to make sure older
clients get an error message that makes sense.
Change-Id: I9e338974e81321660652dd3b866efc0c9a865566
---
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
2 files changed, 48 insertions(+), 59 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/12661/2
--
To view, visit http://gerrit.cloudera.org:8080/12661
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9e338974e81321660652dd3b866efc0c9a865566
Gerrit-Change-Number: 12661
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tserver: remove DEPRECATED range predicates
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12661 )
Change subject: tserver: remove DEPRECATED_range_predicates
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12661/1/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/12661/1/src/kudu/tserver/tablet_service.cc@a1872
PS1, Line 1872:
> Can we somehow enforce that the client is new enough to not be sending thes
Makes sense. Done
--
To view, visit http://gerrit.cloudera.org:8080/12661
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e338974e81321660652dd3b866efc0c9a865566
Gerrit-Change-Number: 12661
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 05 Mar 2019 21:59:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] tserver: remove DEPRECATED range predicates
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has abandoned this change. ( http://gerrit.cloudera.org:8080/12661 )
Change subject: tserver: remove DEPRECATED_range_predicates
......................................................................
Abandoned
--
To view, visit http://gerrit.cloudera.org:8080/12661
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I9e338974e81321660652dd3b866efc0c9a865566
Gerrit-Change-Number: 12661
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tserver: remove DEPRECATED range predicates
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12661 )
Change subject: tserver: remove DEPRECATED_range_predicates
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12661/1/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/12661/1/src/kudu/tserver/tablet_service.cc@a1872
PS1, Line 1872:
Can we somehow enforce that the client is new enough to not be sending these? I'm fine breaking kudu 0.8 client, but we should make sure that if someone uses it, it gets back a nice error message rather than silently ignoring the passed predicates.
One option is to do something like:
if (scan_pb.deprecated_range_predicates_size() > 0) {
return Status::InvalidArgument("Kudu client version 0.8 and older not supported");
}
--
To view, visit http://gerrit.cloudera.org:8080/12661
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e338974e81321660652dd3b866efc0c9a865566
Gerrit-Change-Number: 12661
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 05 Mar 2019 17:43:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] tserver: remove DEPRECATED range predicates
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12661 )
Change subject: tserver: remove DEPRECATED_range_predicates
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12661/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12661/2//COMMIT_MSG@9
PS2, Line 9: This field hasn't been touched in 3 years (Kudu 0.8.0), and would
: otherwise be another piece of a scan to consider for authorization.
: Before moving ahead with authorization, it seems like as good a time as
: any to remove it. Using this field will now result in an error being
: returned to clients.
> Different people have different takes on what deprecated means:
I am not philosophically opposed to camp #2, so I guess I'll leave it. I've made note of this in KUDU-1944.
--
To view, visit http://gerrit.cloudera.org:8080/12661
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e338974e81321660652dd3b866efc0c9a865566
Gerrit-Change-Number: 12661
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 06 Mar 2019 00:48:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] tserver: remove DEPRECATED range predicates
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12661 )
Change subject: tserver: remove DEPRECATED_range_predicates
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12661/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12661/2//COMMIT_MSG@9
PS2, Line 9: This field hasn't been touched in 3 years (Kudu 0.8.0), and would
: otherwise be another piece of a scan to consider for authorization.
: Before moving ahead with authorization, it seems like as good a time as
: any to remove it. Using this field will now result in an error being
: returned to clients.
> I agree that ColumnRangePredicates have been deprecated for a long time, bu
This wouldn't make it "more complex" per se, but if we don't remove this, I will need to write code to handle the field, which feels odd given it's deprecated status.
Three years seems like a long enough sunset period to give users time to move off of it. Otherwise, what's the point of labeling it deprecated in the first place?
--
To view, visit http://gerrit.cloudera.org:8080/12661
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e338974e81321660652dd3b866efc0c9a865566
Gerrit-Change-Number: 12661
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 05 Mar 2019 23:22:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] tserver: remove DEPRECATED range predicates
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12661 )
Change subject: tserver: remove DEPRECATED_range_predicates
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12661/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12661/2//COMMIT_MSG@9
PS2, Line 9: This field hasn't been touched in 3 years (Kudu 0.8.0), and would
: otherwise be another piece of a scan to consider for authorization.
: Before moving ahead with authorization, it seems like as good a time as
: any to remove it. Using this field will now result in an error being
: returned to clients.
> This wouldn't make it "more complex" per se, but if we don't remove this, I
Different people have different takes on what deprecated means:
- For folks who prefer a fast-moving project that aggressively cleans up after itself, it means "this thing is going to be removed soon; please use this other thing instead".
- For folks who prefer infinite backwards compatibility, it means "this thing isn't as featureful/robust/smart as this other thing; our official position is that you use that other thing instead."
As you can see, there can be consensus over something's deprecation status without there being consensus over when/how to actually remove it. Personally, I'm in camp #2, which is how I was able to support the various deprecations we've done over the years without also committing to their removal.
In any case, I would be more open to removal if it happened in the context of a major Kudu release. KUDU-1944 tracks various ideas for other breaking changes we could make in that context.
--
To view, visit http://gerrit.cloudera.org:8080/12661
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e338974e81321660652dd3b866efc0c9a865566
Gerrit-Change-Number: 12661
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 05 Mar 2019 23:44:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] tserver: remove DEPRECATED range predicates
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12661 )
Change subject: tserver: remove DEPRECATED_range_predicates
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12661/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12661/2//COMMIT_MSG@9
PS2, Line 9: This field hasn't been touched in 3 years (Kudu 0.8.0), and would
: otherwise be another piece of a scan to consider for authorization.
: Before moving ahead with authorization, it seems like as good a time as
: any to remove it. Using this field will now result in an error being
: returned to clients.
I agree that ColumnRangePredicates have been deprecated for a long time, but they're still exposed in the Java client as a possible means of setting scan predicates (see AbstractKuduScannerBuilder), so we can't necessarily guarantee that new usages of this haven't sprung up since Kudu 0.8.
AFAIK this would be the first time we've removed public client-facing functionality since Kudu hit 1.0, and as such I think we need to think really hard about whether that's a bridge we want to cross, especially outside of a major release. I generally view such removals as quite user hostile (even if the functionality in question has been deprecated for years), and I'd only support it if it yielded a significant simplification in complexity/implementation. Is that true in this case?
--
To view, visit http://gerrit.cloudera.org:8080/12661
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e338974e81321660652dd3b866efc0c9a865566
Gerrit-Change-Number: 12661
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 05 Mar 2019 23:01:55 +0000
Gerrit-HasComments: Yes