Skip to content
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

Add a WatchStreamExt trait for stream chaining #899

Merged
merged 13 commits into from May 11, 2022
Merged

Add a WatchStreamExt trait for stream chaining #899

merged 13 commits into from May 11, 2022

Conversation

clux
Copy link
Member

@clux clux commented May 8, 2022

From discussion with @teozkr after #698 in december.
This tries to solve the problem in a more generic way using Stream helpers.
A single example using it is included for now. Want to have some reviews before tidying this up properly. ..well, i had free time now, have tidied.

It was not trivial (but followed similar bits from tokio-stream following the StreamExt setup). it might not be the best way to do this, but every other path i tried failed.

EDIT: Note the methods in here were further renamed in #906 before the 0.72.0 release.

failed path

tried a smarter setup without having to reimplement `into_iter_applied` inlined into an `impl Stream` but i just couldn't get the types to line up. the `impl Stream` we get is opaque and cannot be put inside the struct `AppliesGen` ```rust #[pin_project] /// Stream returned by the [`Applies`](super::WatchStreamExt::watch_applies) method. #[must_use = "streams do nothing unless polled"] pub struct AppliesGen { #[pin] stream: St, phantom: PhantomData, } impl AppliesGen where K: Sized + Send + 'static, St: Stream, watcher::Error>> + 'static, { pub(super) fn new(st: St) -> Self { let stream = utils::try_flatten_applied(st); // opaque type :( AppliesGen { stream, phantom: PhantomData } } } impl Stream for AppliesGen where { type Item = Result;

fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<OptionSelf::Item> {
self.as_mut()
.project()
.stream
.poll_next(cx)
}
}

</p>
</details>

From discussion with @teozkr after #698 .
This tries to solve the problem in a more generic way using Stream
helpers.

It was not trivial, and I'm not convinced this is the easiest way to do
it, but every other path i tried failed.

Signed-off-by: clux <sszynrae@gmail.com>
@clux clux added the changelog-add changelog added category for prs label May 8, 2022
@clux clux requested a review from nightkr May 8, 2022 15:27
@codecov-commenter
Copy link

codecov-commenter commented May 8, 2022

Codecov Report

Merging #899 (81ab1db) into master (f3eee32) will increase coverage by 0.25%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master     #899      +/-   ##
==========================================
+ Coverage   70.35%   70.61%   +0.25%     
==========================================
  Files          62       64       +2     
  Lines        4291     4339      +48     
==========================================
+ Hits         3019     3064      +45     
- Misses       1272     1275       +3     
Impacted Files Coverage Δ
kube-runtime/src/utils/mod.rs 17.64% <ø> (ø)
kube-runtime/src/utils/watch_ext.rs 0.00% <0.00%> (ø)
kube-runtime/src/utils/event_flatten.rs 97.77% <97.77%> (ø)
kube-runtime/src/wait.rs 70.00% <0.00%> (+2.00%) ⬆️

clux added 2 commits May 8, 2022 21:04
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@clux clux changed the title Add a WatchStreamExt trait for better event chaining Add a WatchStreamExt trait for stream chaining May 8, 2022
@clux clux added this to the 0.72.0 milestone May 9, 2022
kube-runtime/src/utils/event_flatten.rs Outdated Show resolved Hide resolved
// present a dumb table for it for now. maybe drop the whole watch. kubectl does not do it anymore.
let mut stream = try_flatten_applied(w).boxed();
// present a dumb table for it for now. kubectl does not do this anymore.
let mut stream = watcher(api, lp).watch_applies().boxed();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be pin_mut! rather than boxed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i am using it elsewhere. i just don't want to mix pin into the easy go-to example.

kube-runtime/src/utils/event_flatten.rs Outdated Show resolved Hide resolved
kube-runtime/src/utils/event_flatten.rs Outdated Show resolved Hide resolved
examples/node_watcher.rs Outdated Show resolved Hide resolved
@nightkr
Copy link
Member

nightkr commented May 11, 2022

All in all, minor implementation nits but much happier with the new API!

We should probably either delete the old flatteners, or turn them into (deprecated) aliases for the new trait methods.

clux added 5 commits May 11, 2022 11:08
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Copy link
Member

@nightkr nightkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to kill off try_flatten_* and merge this now then.

@clux
Copy link
Member Author

clux commented May 11, 2022

I'm going to mark those as deprecated (with notice of removal in 0.75.0) and then update all the examples first.
Thanks for the great review!

@nightkr
Copy link
Member

nightkr commented May 11, 2022

Deprecation is good enough to me.

clux added 3 commits May 11, 2022 14:38
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@clux clux merged commit beeeb95 into master May 11, 2022
@clux clux deleted the watch-ext branch May 11, 2022 17:30
///
/// All Added/Modified events are passed through, and critical errors bubble up.
/// This is functionally equivalent to calling [`try_flatten_applied`] on a [`watcher`].
fn watch_applies<K>(self) -> EventFlatten<Self, K>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's just me, but WatchStreamExt::watch_applies and WatchStreamExt::watch_touches sounds kind of awkward.

applied_objects and touched_objects?

watcher(api, lp).applied_objects()
watcher(api, lp).touched_objects()

reflector(store, watcher(api, lp)).applied_objects()
trigger_with(watcher(api, lp).touched_objects(), |obj| {})

Or even just applied and touched?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I personally like those better myself, even the short variants makes sense to me.
if not the short ones, then at the very least the _objects suffix is better than a watch prefix - which is almost implied by the trait.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raised a PR in #906 for this.

clux added a commit that referenced this pull request May 11, 2022
as suggested by #899 (review)

Signed-off-by: clux <sszynrae@gmail.com>
clux added a commit that referenced this pull request May 11, 2022
* Rename unreleased watcher ext methods slightly

as suggested by #899 (review)

Signed-off-by: clux <sszynrae@gmail.com>

* fix forgotten references to old method in docs

Signed-off-by: clux <sszynrae@gmail.com>

* accidental private

Signed-off-by: clux <sszynrae@gmail.com>

* ugh fmt reorder

Signed-off-by: clux <sszynrae@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-add changelog added category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants