Skip to content
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

Merged
merged 4 commits into from
May 18, 2023

Conversation

zasweq
Copy link
Contributor

@zasweq zasweq commented May 15, 2023

Fixes #6273. This PR switches Outlier Detection to use the new duration field for correct JSON -> internal parsing.

RELEASE NOTES: N/A

@zasweq zasweq requested a review from dfawley May 15, 2023 23:43
@zasweq zasweq added the Type: Internal Cleanup Refactors, etc label May 15, 2023
@zasweq zasweq added this to the 1.56 Release milestone May 15, 2023
Comment on lines 755 to 757
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))))
Copy link
Member

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.Durations 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?
}

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Comment on lines -162 to -163
"interval": 50000000,
"baseEjectionTime": 100000000,
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@dfawley dfawley assigned zasweq and unassigned dfawley May 16, 2023
@zasweq zasweq merged commit 098b2d0 into grpc:master May 18, 2023
1 check passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch Outlier Detection to use new duration field
2 participants