- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 365
 
better docs and links between for Api::watch and watcher #377
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
Conversation
| /// ``` | ||
| /// [`try_flatten_applied`]: super::utils::try_flatten_applied | ||
| /// [`reflector`]: super::reflector::reflector | ||
| /// [`Api::watch`]: https://docs.rs/kube/*/kube/struct.Api.html#method.watch | 
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 thought rustdoc was supposed to resolve these links automagically these days
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 wasn't able to get a cross-crate link to work :/
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.
Seems to work for tokio-util (https://docs.rs/tokio-util/0.6.0/src/tokio_util/context.rs.html#22).. which direction did you test in? Makes sense that it would be tricker for links from kube -> kube-runtime than the other way around.
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.
They have the tokio::runtime::Runtime in scope 7 lines up. If we ever re-export kube_runtime from kube under a runtime feature (which I think we should aim to do), then within crate links will work from kube -> kube_runtime.
The other way around has the same problem for the time being.
        
          
                kube/src/api/typed.rs
              
                Outdated
          
        
      | /// Note that a watch call __will terminate in <5 minutes__, based on [`ListParams::timeout`], | ||
| /// and as such, is best suited for __short term__ watches. | ||
| /// For continual tracking of resources, consider a managed [`watcher`]. | 
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.
- The timeout parameter is just a hint to the server, which can really enforce whatever timeout it wants, so (in the long term) there isn't really a reliable threshold here for when 
Api::watchis safe - There are other causes for errors here (network interruption, apiserver bugs, network topology changes, restarts), that you'd probably also want to recover from
 
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.
Yeah, that is true, probably need to go through #334 and check how this interacts without defaults now. We should possibly remove the hard limit.
You are right. I'll reword it a bit. Possibly we should have some more convenience (or at least examples) around watchers that allow the same type of watch with an explicit timeout (without having to consider the other error cases).
Co-authored-by: Teo Klestrup Röijezon <teo.roijezon@interops.se>
Co-authored-by: Teo Klestrup Röijezon <teo.roijezon@interops.se>
Co-authored-by: Teo Klestrup Röijezon <teo.roijezon@interops.se>
also remove broken inferred link to TryFuture*
trying to make it clear that a straight
Api::watchis not recommended for anything but <a few minute watch times