-
-
Notifications
You must be signed in to change notification settings - Fork 301
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
Rename new watcher Event names and remove one that cannot happen #1499
Conversation
1. Event::Init now marks start, it is followed by one/more of: 2. Event::InitApply or EventInitPage 3. Event::Ready marks init complete removed Event::RestartDelete because by docs it cannot happen: https://kubernetes.io/docs/reference/using-api/api-concepts/#streaming-lists have inserted a big error in case they break their own docs for this alpha feature. Signed-off-by: clux <sszynrae@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1499 +/- ##
=======================================
+ Coverage 74.9% 75.0% +0.1%
=======================================
Files 78 78
Lines 6864 6854 -10
=======================================
- Hits 5136 5134 -2
+ Misses 1728 1720 -8
|
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.
I don't love how this imposes the mental overhead of pagination on all clients (I use raw watchers frequently-ish when I "just" want to build a local cache for some purpose.. they would probably benefit from pagination but building that isn't free :p), but I suppose that's kind of unavoidable without us having to maintain both paginated and buffered interfaces.
/// A page of objects was received during the restart. | ||
/// If using the `ListWatch` strategy, this is a relist, and indicates that `InitlPage` events will follow. | ||
/// If using the `StreamingList` strategy, this event will be followed by `InitApply` + `InitDelete` events. | ||
Init, |
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.
Do we actually gain much from having this be a separate variant, or would it be enough to say that InitPage
/InitApply
/Ready
implies the start of an init if one is not already happening?
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.
You could do it either way, but it feels slightly cleaner to have an event for it where you can clear any temporary buffer. Otherwise you'd have to check if it's the first page, and only do it then in an if
.
...and you also have to do that check in two places if you need to support both ListStrategy
(since InitPage
and InitApply
have different signatures). EDIT: unless your suggestion below works out.
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.
I think it works out kind of naturally to call buffer.or_insert_default()
in InitPage
and buffer.take()
in InitReady
(where buffer: Option<_>
), but it's a fair point.
InitPage(Vec<K>), | ||
/// An object received during `Init` with the `StreamingList` strategy. | ||
/// | ||
/// Any objects that were previously [`Applied`](Event::Applied) but are not listed in any of the pages | ||
/// should be assumed to have been [`Deleted`](Event::Deleted). | ||
RestartPage(Vec<K>), | ||
/// An object was added or modified during the initial watch. | ||
/// | ||
/// This event is only returned when using the `StreamingList` strategy. | ||
RestartApply(K), | ||
/// An object was deleted during the initial watch. | ||
/// | ||
/// This event is only returned when using the `StreamingList` strategy. | ||
RestartDelete(K), | ||
/// The restart is complete. | ||
/// Any objects that were previously [`Applied`](Event::Applied) but are not listed in any of | ||
/// the `InitAdd` events should be assumed to have been [`Deleted`](Event::Deleted). | ||
InitApply(K), |
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.
These could be merged into one InitPage(SmallVec<[K; 1]>)
, I think? (Using https://docs.rs/smallvec/latest/smallvec/struct.SmallVec.html)
That way, only users that actually care about the distinction have to care, while you still avoid the allocation overhead if it isn't needed.
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.
Another option would be to extract the pagination strategy to a trait/generic.
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.
That should be possible yes. My only worry about merging the enum variants is that InitPage
can go away (in like 3 years) when StreamingList
is stabilised and covered in our MK8SV, at which point it's not necessary to have a vector there at all.
The vector part on the receiver is only slightly more verbose though
kube/kube-runtime/src/reflector/store.rs
Lines 114 to 125 in 59efd8d
watcher::Event::InitPage(new_objs) => { | |
let new_objs = new_objs | |
.iter() | |
.map(|obj| (obj.to_object_ref(self.dyntype.clone()), Arc::new(obj.clone()))) | |
.collect::<AHashMap<_, _>>(); | |
self.buffer.extend(new_objs); | |
} | |
watcher::Event::InitApply(obj) => { | |
let key = obj.to_object_ref(self.dyntype.clone()); | |
let obj = Arc::new(obj.clone()); | |
self.buffer.insert(key, obj); | |
} |
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.
I think extracting the strategy into a trait somehow could be useful for many reasons though! But would like to iron out deficiencies in shared streams controllers firsts.
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.
You shouldn't need to collect into the AHashMap
fwiw, Extend::extend
takes an iterator rather than a concrete collection. So the smallveced version would end up as "just":
watcher::Event::InitPage(new_objs) => {
let new_objs = new_objs
.iter()
.map(|obj| (obj.to_object_ref(self.dyntype.clone()), Arc::new(obj.clone()))) ;
self.buffer.extend(new_objs);
}
Alternatively, if we want to lean into InitApply
as the path forward, we could also always flatten the list (so emit multiple InitApply
when we receive an InitPage
event).
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.
TIL. Extend trick applied: 4f69a37 👍
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.
Alternatively, if we want to lean into InitApply as the path forward, we could also always flatten the list (so emit multiple InitApply when we receive an InitPage event).
Possibly, but then we are introducing buffering again inside the trampoline so that we can loop through the vector.
This does feel a bit like an optimisation, and I wonder if this, along with your other use case you are describing in your main review comment can be minimised instead with an additional reducer ala Event::into_old_style_event_something()
that can do some bare-bones buffering for your use case?
InitApply should be propagated here, pages are. Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
The clear improvement suggestions herein are applied (have some concerns on |
merging this as is for now, but will try to merge |
Follow-up to the unreleased #1494. Make names more easy to read and clear which one signifies start and which is end:
Event::Init
now marks start, it is followed by one/more of:Event::InitApply
xorEventInitPage
Event::InitDone
marks init completeremoved
Event::RestartDelete
because by docs it cannot happenhttps://kubernetes.io/docs/reference/using-api/api-concepts/#streaming-lists
have inserted a big error in case they break their own docs for this alpha feature.