-
Notifications
You must be signed in to change notification settings - Fork 210
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
chore: improve error of download config #4603
Conversation
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.
Certainly an improvement, but candidate for further refactoring
ea440fb
to
0dc05ee
Compare
0dc05ee
to
433d53e
Compare
- use it in download client config
433d53e
to
da6cca6
Compare
now returns the correct error with added context
da6cca6
to
7c135da
Compare
op_name: impl Into<String>, | ||
op_fn: F, | ||
interval: Duration, | ||
max_attempts: u32, |
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.
max_attempts < timeout , just saying.
Isn't this poll
from devimint
where I just added deadlines and turned interval to be growing? Can we move it here from there and rename?
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 one uses ControlFlow
, while here we want that to be implicitly implied by Result
. retry
could be implemented using poll
though.
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 control flow there is a bit of a PITA, BTW (because of type inference on two variants). Not sure how badly we need it. In testing if there's an error that should really terminate polling... maybe we should just panic, instead of complicating common case? Just thinking aloud.
But one way or the other - would prefer to see a timeout/deadline approach here.
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.
Will leave it to @maan2003, don't have a strong opinion here.
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.
🤷 devimint and this retry can just stay separate
attempt + interval is pretty close to deadline though
Let's land it already, doesn't matter all that much in a great scheme of things, Manmeet is free to follow up (or not). :D |
now returns the correct error with added context