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 WatchStreamExt::default_backoff shorthand #1232

Merged
merged 7 commits into from
Jun 30, 2023
Merged

Add WatchStreamExt::default_backoff shorthand #1232

merged 7 commits into from
Jun 30, 2023

Conversation

clux
Copy link
Member

@clux clux commented Jun 28, 2023

This change adds a simplified recommended backoff construction for watcher streams:

-let stream = watcher(api, config).backoff(watcher::default_backoff()).
+let stream =  watcher(api, config).default_backoff();

This is advantageous because it's hard to advocate that users use backoff (fundamentally a good behaviour action) when the usage is verbose. So this makes it less verbose.

To do this properly we have made a new DefaultBackoff struct that encapsulates the type. This is slightly more verbose than relying on impl Backoff from watcher::default_backoff, but it has the benefit of working in the stream setting where we are limited by existential type constraints.

This also deprecates the watcher::default_backoff fn due to it now generally being more easily invoked elsewhere, and by the explicit type. This could be debated.

old, now irrelevant commentary, left in a collapsible

The slight downside here is that `watcher::default_backoff()` returns an opaque type and cannot be used in the shortcut stream ext method so have exposed just our `ExponentialBackoff` default as an internal fn (which cannot be const), along with a const reset timeout and re-implemented the function in `WatchStreamExt::default_backoff` referencing our parameters.

An alternative could be doing the defaulting in the generally opaque type that most users don't interact with:

// convenience for watchstreamext users
impl Default for ResetTimerBackoff<backoff::ExponentialBackoff> {
    fn default() -> Self {
        use crate::watcher::{default_exponential_backoff, DEFAULT_RESET_DURATION};
        let bo = default_exponential_backoff();
        Self::new(bo, DEFAULT_RESET_DURATION)
    }
}

but i think that doesn't really help much. we still end up with an implementation in WatchStreamExt that is almost as big (except without the internal imports) and it's an awkward inversion of utils depending on core stuff. I'd probably rather keep the backoff parameters somewhere prominent.

clux added 2 commits June 28, 2023 16:09
To allow the same chaining style without having to import all the backoff dependencies directly.
This allows me to write more concise recommendation documentation in kube-rs/website.

Signed-off-by: clux <[email protected]>
@clux clux added the changelog-add changelog added category for prs label Jun 28, 2023
@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

Merging #1232 (7bbf028) into main (a6a627c) will decrease coverage by 0.13%.
The diff coverage is 0.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1232      +/-   ##
==========================================
- Coverage   72.06%   71.94%   -0.13%     
==========================================
  Files          71       71              
  Lines        5714     5725      +11     
==========================================
+ Hits         4118     4119       +1     
- Misses       1596     1606      +10     
Impacted Files Coverage Δ
kube-runtime/src/controller/mod.rs 34.00% <0.00%> (ø)
kube-runtime/src/utils/watch_ext.rs 0.00% <0.00%> (ø)
kube-runtime/src/watcher.rs 39.31% <0.00%> (-2.92%) ⬇️

... and 1 file with indirect coverage changes

@clux clux marked this pull request as ready for review June 28, 2023 15:59
@clux clux added this to the 0.84.0 milestone Jun 28, 2023
@Dav1dde
Copy link
Member

Dav1dde commented Jun 28, 2023

Existential types would be nice ...

I like the helper, especially since this is already the controller default behaviour.

Have you considered just encapsulating the default backoff in a new-type?

pub struct DefaultBackoff(InnerStrategyNotPublic)

impl Default for DefaultBackoff {
    fn default() -> Self {
        Self(default_exponential_backoff())
    }
}

impl Backoff for DefaultBackoff {
    fn next_backoff(&mut self) -> Option<Duration> {
        self.0.next_backoff()
    }
}

And then just expose DefaultBackoff in all the APIs (including the default_backoff()) function.

@clux
Copy link
Member Author

clux commented Jun 28, 2023

Have you considered just encapsulating the default backoff in a new-type?

pub struct DefaultBackoff(InnerStrategyNotPublic)

And then just expose DefaultBackoff in all the APIs (including the default_backoff()) function.

That suggestion does have legs. I might change the naming a bit because you'd end up with impl Default for DefaultBackoff and you'd construct it with DefaultBackoff::default() which feels awkward. Perhaps watcher::Backoff::default() and then deprecating the old default_backoff fn will be helpful here.

EDIT: no, watcher::Backoff is probably a bad name given there are so many different Backoff suffixed things. 🤔

@clux
Copy link
Member Author

clux commented Jun 28, 2023

Ok, have done this properly now. It is still somewhat hampered by the fact that ResetTimerBackoff is actually public, but have at least managed to keep that type out of DefaultBackoff and the WatchStreamExt!

kube-runtime/src/watcher.rs Outdated Show resolved Hide resolved
kube-runtime/src/watcher.rs Outdated Show resolved Hide resolved
kube-runtime/src/watcher.rs Outdated Show resolved Hide resolved
kube-runtime/src/watcher.rs Show resolved Hide resolved
@clux
Copy link
Member Author

clux commented Jun 30, 2023

Giving a ping to @nightkr here in case of extra opinions since she wasn't cc'd on originally.

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.

LGTM

@clux clux merged commit f792fd2 into main Jun 30, 2023
@clux clux deleted the backoff_ext branch June 30, 2023 22:45
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.

3 participants