Add new classes for eliminating xds config tears#11740

Merged

larry-safran merged 26 commits intogrpc:masterfrom

larry-safran:XdsDependencyManager

Feb 8, 2025

Contributor

larry-safran commented Dec 11, 2024 •

edited

Loading

This is just defining the XdsDependencyManager and associated classes; no movement of any functionality.

larry-safran requested a review from ejona86

December 11, 2024 02:56

larry-safran marked this pull request as draft

December 11, 2024 02:56

ejona86 reviewed

View reviewed changes

Member

ejona86 left a comment

The shape looks fine.

xds/src/main/java/io/grpc/xds/XdsClusterSubscriptionRegistry.java Outdated Show resolved Hide resolved

larry-safran force-pushed the XdsDependencyManager branch from 32c80c0 to f995211 Compare

December 14, 2024 00:50

larry-safran force-pushed the XdsDependencyManager branch 2 times, most recently from f5090ab to 9fcbe4c Compare

January 6, 2025 22:13

linux-foundation-easycla bot commented Jan 7, 2025 •

edited

Loading

The committers listed above are authorized under a signed CLA.

✅ login: larry-safran / name: Larry Safran (5f8d479, 4a53fce, 06466fc, c97118f, b2cb05b, 954ced3, 5b14a3e, ec50490, 5dceeaf, d3b713f, d5dea83, 6089730, fd069e3, fd64f20, ef13712, 8e668b5, 68071e6, 28d29fb, b2e924e, 5a75b10, f99fc56, b898e34, 482cd9d, 7042ed9, 518cef1, 36602b6)

larry-safran marked this pull request as ready for review

January 7, 2025 05:35

larry-safran requested a review from ejona86

January 7, 2025 05:35

Contributor Author

larry-safran commented Jan 7, 2025

Ready for review @ejona86

ejona86 reviewed

View reviewed changes

Member

ejona86 left a comment

I still have a good amount to review. Sending what I have.

xds/src/main/java/io/grpc/xds/XdsConfig.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsConfig.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsConfig.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsRouteConfigureResource.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsConfig.java Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated Show resolved Hide resolved

ejona86 reviewed

View reviewed changes

Member

ejona86 left a comment

Nothing really new here; just a quick drive-by. Still need to look through the other parts of the PR.

xds/src/main/java/io/grpc/xds/XdsConfig.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsConfig.java Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsConfig.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated Show resolved Hide resolved

larry-safran force-pushed the XdsDependencyManager branch from 1fac607 to 3f59a16 Compare

January 8, 2025 19:41

larry-safran requested a review from ejona86

January 14, 2025 00:47

larry-safran added 13 commits

January 13, 2025 17:35


          Framework definition to support A74

fd069e3


          Test that update works (with associated fixes)

b2cb05b


          Cleanup

8e668b5


          Cleanup

5b14a3e


          Add verification of data changing

5f8d479


          Support aggregate clusters correctly

ec50490


          Fix class name referenced in javadoc

5dceeaf


          Address a number of code review comments and add a test for missing C…

fd64f20

…ds aggregate and eds entries.


          Remove syncContext from watchers. Add checkNotNull, private and final…

b2e924e

… as recommended by code review.


          Add a test for corrupt LDS

5a75b10


          Change aggregate cluster handling to correctly handle cluster names a…

d3b713f

…ppearing in multiple trees or overlapping with the RDS.virtualHosts.


          Errorprone


          Add max recursion limit for clusters to match c++.

d5dea83

larry-safran force-pushed the XdsDependencyManager branch from 32a2975 to d5dea83 Compare

January 14, 2025 02:04

ejona86 reviewed

View reviewed changes

Member

ejona86 left a comment

Found these comments pending. Sending

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated Show resolved Hide resolved

ejona86 reviewed

View reviewed changes

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated Show resolved Hide resolved

larry-safran added 2 commits

January 15, 2025 13:16


          Fix handling of route and cluster updates.

28d29fb


          Make data private and XdsWatcherBase static

4a53fce

ejona86 reviewed

View reviewed changes

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated Show resolved Hide resolved

larry-safran added 9 commits

January 16, 2025 13:22


          In LDS onChanged(), get old activeVirtualHost before possibly doing c…

c97118f

…leanUpRdsWatcher().

Let cancelWatcher remove the old RDS watcher instead of doing it explicitly in cleanUpRdsWatcher.


          Change comment for clarity

06466fc


          Allow EdsWatcher to have multiple CdsWatcher parents

b898e34


          Allow EdsWatcher to have multiple CdsWatcher parents

954ced3


          Fully support inlined RouteConfig

ef13712


          Add lots of checkNotNull()

518cef1


          Add test case testMultipleParentsInCdsTree, make a couple of cluster …

68071e6

…related fixes and add some test utility methods for generating xds configuration.


          Add test case testMultipleParentsInCdsTree, make a couple of cluster …

f99fc56

…related fixes and add some test utility methods for generating xds configuration.


          Add virtual host to XdsConfig as per spec

482cd9d

ejona86 reviewed

View reviewed changes

Member

ejona86 left a comment

Apparently I didn't send these comments out yesterday. (I'm still going through it, but you can work on these in parallel.)

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated Show resolved Hide resolved

larry-safran added 2 commits

January 31, 2025 17:54


          Eliminate clusterSubscriptions and use objects directly instead of st…

36602b6

…rings for cluster watcher parents.


          Add tests. Fix some places that were still using string instead of ob…

7042ed9

…ject for parent.

Don't send an update if the configuration hasn't changed.

ejona86 approved these changes

View reviewed changes

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java

+                    XdsWatcherBase<?> cdsWatcher =
+                        resourceWatchers.get(CLUSTER_RESOURCE).watchers.get(clusterName);
+                    if (cdsWatcher == null) {
+                      return; // already released while waiting for the syncContext

Member

ejona86 Feb 7, 2025

This comment may be out-of-date. Since the subscription hasn't been removed from parent sets, this would only be possible if releaseSubscription() had already been called for this subscription (which is up to you whether is an error or not).

larry-safran merged commit 67fc2e1 into grpc:master

16 checks passed

github-actions bot locked as resolved and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet