-
Notifications
You must be signed in to change notification settings - Fork 40k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Alpha list paging implementation #48921
Alpha list paging implementation #48921
Conversation
ff417b5
to
aa72108
Compare
var options []clientv3.OpOption | ||
switch { | ||
case len(fromKey) > 0 && pred.Limit > 0: | ||
options = []clientv3.OpOption{clientv3.WithRev(fromRV), clientv3.WithRange(key + "\xFF"), clientv3.WithLimit(pred.Limit)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xiang90 if I want to prevent seeing updates newer than fromRV
, what do I filter by?
28ccce1
to
7bf2504
Compare
97f5102
to
adac573
Compare
0c5c699
to
34c716d
Compare
cc @jpbetz |
I'm still working up to testing this against our mega clusters to see if it can help bring in long tail latency. It also potentially lets us do flyweight caches more effectively (like for GC) |
34c716d
to
eb80234
Compare
9fc8c0e
to
d4e439b
Compare
My only comment is about the comment saying that it's in order of fifteen minutes or so, I would like to get rid of it. But I'm not going to block on it. @smarterclayton please fix the verify script and I'm happy to lgtm it then. |
d4e439b
to
aac7182
Compare
I really really want to have a rough bound on time, we can always adjust it up or make it differently. But it's going to be 5 ootb and I don't want people assuming it is forever. |
That one I completely agree with. I just didn't want to mention "five to fifteen minutes" in the comment. But well, let's not waste too much time on this discussion. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smarterclayton, wojtek-t Associated issue: 896 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/retest |
1 similar comment
/retest |
@smarterclayton - this requires a rebase (not sure why bot didn't mention it). |
Add a feature gate in the apiserver to control whether paging can be used. Add controls to the storage factory that allow it to be disabled per resource. Use a JSON encoded continuation token that can be versioned. Create a 410 error if the continuation token is expired. Adds GetContinue() to ListMeta.
Adds a `continue` and `limit` parameter to ListOptions
aac7182
to
9b8e42a
Compare
Trivial rebase |
/retest |
Automatic merge from submit-queue (batch tested with PRs 50832, 51119, 51636, 48921, 51712) |
Automatic merge from submit-queue (batch tested with PRs 48552, 51876) Disable default paging in list watches For 1.8 this will be off by default. In 1.9 it will be on by default. Add tests and rename some fields to use the `chunking` terminology. Note that the pager may be used for other things besides chunking. Follow on to #48921, we left the field on to get some exercise in the normal code paths, but needs to be disabled for 1.8. @liggitt let's merge on wednesday.
Design in kubernetes/community#896
Support
?limit=NUMBER
,?continue=CONTINUATIONTOKEN
, and acontinue
fieldon ListMeta and pass through to etcd. Perform minor validation as an example.