-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
xds/internal/balancer/outlierdetection: Switch Outlier Detection to use new duration field #6286
Conversation
et := time.Duration(b.cfg.BaseEjectionTime).Nanoseconds() * addrInfo.ejectionTimeMultiplier | ||
met := max(time.Duration(b.cfg.BaseEjectionTime).Nanoseconds(), time.Duration(b.cfg.MaxEjectionTime).Nanoseconds()) | ||
curTimeAfterEt := now().After(addrInfo.latestEjectionTimestamp.Add(time.Duration(min(et, met)))) |
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.
Do you need to "convert" to nanos and back? Why? This should work, I think:
et := b.cfg.BaseEjectionTime * addrInfo.ejectionTimeMultiplier
met := b.cfg.BaseEjectionTime + b.cfg.MaxEjectionTime
curTimeAfterEt := now().After(addrInfo.latestEjectionTimestamp.Add(time.Duration(min(et, met)))
min
might need to change; change it or make another min
that operates on isvccfg.Duration
s if you have other needs for an int64 verison of min
.
Also, I think this might be even clearer:
ejectAfter := b.cfg.BaseEjectionTime * addrInfo.ejectionTimeMultiplier
if max := b.cfg.BaseEjectionTime + b.cfg.MaxEjectionTime; ejectAfter > max {
ejectAfter = max
} // or use `min` from above if you prefer; I don't have any preference.
ejectTime := addrInfo.latestEjectionTimestamp.Add(time.Duration(ejectAfter)) // explains the value after math
if now().After(ejectTime) { // instead of storing bool then checking it
b.unejectAddress() // uneject after ejection time?
}
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'd prefer keeping the logic as is since this more closely aligns with the language from the gRFC describing the algorithm. However, I do like your suggestion about the variable names (i.e. ejectAfter and ejectTime). I will change to those.
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.
Oh wait, var names only apply to your suggested. Keeping it, but switching min and getting rid of conversions.
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.
Oh, I remembered why I converted, you can't multiply a time.Duration * int. Needs to be converted to nanos to have that operation make sense.
"interval": 50000000, | ||
"baseEjectionTime": 100000000, |
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.
Can you normalize these in case we decide to be more restrictive in Duration parsing one day, we won't need to come back and change these?
E.g. "0.050s"
- 3/6/9 digits after decimal & don't omit leading digit.
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.
Done.
Fixes #6273. This PR switches Outlier Detection to use the new duration field for correct JSON -> internal parsing.
RELEASE NOTES: N/A