Ensure that initial events are sorted for WatchList #120897
Conversation
k8s-ci-robot
added
release-note-none
|
/assign @p0lyn0mial |
k8s-ci-robot
added
approved
|
/retest |
k8s-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MadhavJivrajani, wojtek-t The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 4a09b2510c8147dbb60218c149a06c24fa1eef88
|
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
| @@ -140,6 +155,7 @@ func newCacheIntervalFromStore(resourceVersion uint64, store cache.Indexer, getA | |||
| } | |||
| buffer.endIndex++ | |||
| } | |||
| sort.Sort(sortableWatchCacheEvents(buffer.buffer)) | |||
There was a problem hiding this comment.
nit: consider using the newer slices.Sort function, which runs faster.
There was a problem hiding this comment.
@wojtek-t mind checking if slices.Sort is faster ?
There was a problem hiding this comment.
I see that it's not used anywhere in the codebase yet - we should do better analysis and migrate more if it appears better.
wojtek-t
force-pushed
the
fix_order_of_init_events
branch
from
fc83ad1
to
0b48036
Compare
k8s-ci-robot
removed
the
lgtm
| } | ||
|
|
||
| func (s sortableWatchCacheEvents) Less(i, j int) bool { | ||
| return s[i].Key < s[j].Key |
There was a problem hiding this comment.
is key = namespace/name ?
There was a problem hiding this comment.
it's the etcd key [that etcd is using to sort, as well as we're using elsewhere:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/storage/cacher/watch_cache.go#L479-L491
]
There was a problem hiding this comment.
OK - I think I found it -
| func (s sortableWatchCacheEvents) Swap(i, j int) { | ||
| s[i], s[j] = s[j], s[i] | ||
| } | ||
|
|
||
| // newCacheIntervalFromStore is meant to handle the case of rv=0, such that the events |
There was a problem hiding this comment.
nit: update the doc saying the output will be sorted.
| @@ -1264,22 +1264,22 @@ func RunWatchSemantics(ctx context.Context, t *testing.T, store storage.Interfac | |||
| // after adding the initial pods which is then used to establish a new watch request | |||
| useCurrentRV bool | |||
|
|
|||
| // The test heavily relies on the fact that initialPods in created | |||
There was a problem hiding this comment.
I think that after your change the tests should not rely on that, right ? Should we change the test cases so that we add some variation ?
There was a problem hiding this comment.
TBH - I completely can't remember why I added this...
Isn't it your PR that is doing that?
There was a problem hiding this comment.
I think you can remove the comment. Since the initial events are read from the map the result will always be random - unless we sort it which is exactly what this PR does. I updated/improved RunWatchSemanticInitialEventsExtended.
wojtek-t
force-pushed
the
fix_order_of_init_events
branch
from
0b48036
to
c52e76b
Compare
wojtek-t
force-pushed
the
fix_order_of_init_events
branch
from
c52e76b
to
92bdc7b
Compare
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 37104cba1d7524979cf7b99eebd23f14f754e4bd
|
Ref kubernetes/enhancements#3157
/kind feature
/priority important-longterm
/sig api-machinery