Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Backoff when no successful schedules #2102
Backoff when no successful schedules #2102
Changes from all commits
ef4677a
926782c
148635d
beb00e3
bb110db
b3b0b2f
b49d9b7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 function can simply return whether to backoff or not. Or from the scheduler's perspective: whether it successfully scheduled anything.
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.
Is a bool better than a specific type? I defined this type so that the API would be self-explanatory (and harder to mistakenly flip the return type)
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.
We can use bools when there is no ambiguity of what the return value is. But in this case, it is a bit ambiguos.
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.
@gabesaba Do you have more advantages rather than using
bool
?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.
For type safety: to make UntilWithBackoff harder to use incorrectly. When users read code, it will be clear that this return value means to backoff or not, rather than having to look up in library definition of UntilWithBackoff when true/false should be returned.
Especially since there's some indirection in the usage:
Scheduler.schedule
implements the API, and this function is provided toUntilWithBackoff
byScheduler.Start
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.
This is interesting, I didn't see this used before. Usually we use fakeClocks. I guess one advantage of a fake clock is that you don't need to wait at all, but given these times are short I think it is ok.
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.
But if we can use the fake clock instead of this mock function, I would prefer to use it.
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.
Note that I call Timer.Reset(0) on the real timer, so there is no waiting. I am open to using a fake clock, but I suppose I will still need to implement some functionality to capture the history (as I did with SpyTimer)?
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.
Cool.
Mostly in the tests I saw we are just using sleep to pass the arbitrary amount of time, like here.
Maybe fakeClock is more appropriate to test at the Scheduler level, where the internal details are more encapsulated. By analogy to the Job controller we could keep the clock at the Scheduler level, and use real for prod, but fake for testing.
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, not quite the same purpose as the fake clock.
But why do we even keep a timer inside the SpyTimer? It seems rather unused.
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.
To avoid implementing the rest of the clock.Timer interface