-
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
completely delete WatchListener and WatchRouteConfig APIs #6849
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #6849 +/- ##
==========================================
+ Coverage 83.43% 83.54% +0.10%
==========================================
Files 286 286
Lines 30801 30781 -20
==========================================
+ Hits 25699 25716 +17
+ Misses 4028 3999 -29
+ Partials 1074 1066 -8
|
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.
Some nits.
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.
LGTM overall just a few final nits.
// This route configuration watcher registers two | ||
// more watches from the OnUpdate callback of the original resource for which it was created. |
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.
Wrap to 80 chars please :).
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.
"registers two watches corresponding to the names passed in at creation time on a valid update"?
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.
// Create a route configuration watcher that registers two more watches from | ||
// the OnUpdate callback: | ||
// - one for the same resource name as this watch, which would be | ||
// satisfied from xdsClient cache | ||
// - the other for a different resource name, which would be | ||
// satisfied from the server |
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 don't understand what this is testing (I feel like a certain xDS resource can't trigger another watch for the same xDS resource) but since this is already in the codebase I'm fine leaving 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.
I feel like a certain xDS resource can't trigger another watch for the same xDS resource
Wouldn't happen in practice. But this test wants to ensure that even if that happens, things work fine.
This PR gets rid of the old
WatchListener
andWatchRouteConfig
APIs. This should be the last PR to call time on the old APIs.#resource-agnostic-xdsclient-api
RELEASE NOTES: none