-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
LifeTimeWatcher SleepDuration calculation testing #17919
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.
@Freyert, thanks so much for submitting this, apologies it's taken a while to get anyone in front of it. I added a few comments, if you could take a look it would be greatly appreciated.
better to hide the implementation detail inside the function
and correct comment
I'm going to commit my suggestions so this can be merged, as we aren't working/looking at this now. Apologies if you didn't get time to review them yet. |
One component that makes fixing #16439 difficult is the complexity of the
doRenewWithOptions
function.Modifying the sleep duration calculation function is a critical aspect of #16439. We need to ensure that if we change the sleep duration calculating to include the
Age-Header
the invariant "sleep duration is less than remaining lease duration" holds true. (see #16439 (comment))I purposely don't use the full calculation in the test because that may be prone to modification in the future, but in all cases the sleep duration should be less than the remaining lease duration.