-
Notifications
You must be signed in to change notification settings - Fork 4k
xds: implement XdsClient APIs for watching LDS/RDS resources individually #7470
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: implement XdsClient APIs for watching LDS/RDS resources individually #7470
Conversation
472087d to
016f871
Compare
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.
Posted diff between XdsClientImpl and XdsClientImpl2 at 1323354 for convenience:
https://gist.github.com/dapengzhang0/0852cc0506fea3afbfd34bfdf9954fb8
| new CancellationListener() { | ||
| @Override | ||
| public void cancelled(Context context) { | ||
| adsEnded.set(true); |
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.
This adsEnded flag is weak because if retry happens there could be multiple streams created, as long as one of them is cancelled, this flag is true. (XdsClientImplTest1 has adsEnded.set(false))
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.
You are right. Added.
…ally (grpc#7470) Add XdsClient implementation of watching LDS/RDS resources, replacing the ConfigWatcher API. This makes LDS/RDS/CDS/EDS resource watchers work similarly. This change also cleans up XdsClientImpl's tests.
First take for replacing ConfigWatcher with LdsResourceWatcher and RdsResourceWatcher. The next take is #7469.
Effective changes should be for LDS/RDS protocols only. Tests for XdsClientImpl are cleaned up.