-
Notifications
You must be signed in to change notification settings - Fork 4.6k
xds: avoid log spam during server mode switches (better A36 compliance) #5215
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
Conversation
|
@dfawley : I made the change we discussed offline to register a callback to log the mode changes if the user did not register any. And therefore, we can directly invoke the callback without a |
9e591f0 to
f563be3
Compare
|
@dfawley : Ping ... |
xds/server.go
Outdated
| // Note that this means that `s.opts.modeCallback` will never be nil and can | ||
| // safely be invoked directly from `handleServiceModeChanges`. | ||
| if so.modeCallback == nil { | ||
| so.modeCallback = func(addr net.Addr, args ServingModeChangeArgs) { |
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.
Make this a standalone function?
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.
xds/server.go
Outdated
| // | ||
| // Note that this means that `s.opts.modeCallback` will never be nil and can | ||
| // safely be invoked directly from `handleServiceModeChanges`. | ||
| if so.modeCallback == nil { |
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.
Should we have a defaultServerOptions that has this set in it, such that if applying the options doesn't override it, we get the defaults? (e.g. how we handle DialOptions in the grpc package)
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.
|
"Suppress redundant mode changes in the listenerWrapper" doesn't sound right. If things are continuing to fail, we need to continue to notify. This is because 1) the error may have changed and 2) it provides a timestamp of the last backoff attempt. The only time to "suppress redundant mode changes" is in the successful case where we are listening and the new mode is still listening. |
Summary of changes:
listenerWrapperTest changes:
t.Errorf()witht.Fatalf()Fixes #4939
RELEASE NOTES: none