Add new classes for eliminating xds config tears #11740
Conversation
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
larry-safran
force-pushed
the
XdsDependencyManager
branch
from
b7681c6 to
f5090ab
Compare
larry-safran
force-pushed
the
XdsDependencyManager
branch
from
f5090ab to
9fcbe4c
Compare
|
Ready for review @ejona86 |
| /** | ||
| * Represents the xDS configuration tree for a specified Listener. | ||
| */ | ||
| public class XdsConfig { |
|
|
||
| XdsConfig o = (XdsConfig) obj; | ||
|
|
||
| return Objects.equals(listener, o.listener) && Objects.equals(route, o.route) |
There was a problem hiding this comment.
You probably want to include the hashCode and have it be the first equal check to short-circuit the more expensive stuff in the most common not-equals case.
| StringBuilder builder = new StringBuilder(); | ||
| builder.append("XdsConfig{listener=").append(listener) | ||
| .append(", route=").append(route) | ||
| .append(", clusters={").append(clusters).append("}}"); |
There was a problem hiding this comment.
clusters.toString() will include the {}.
| @@ -136,7 +136,7 @@ protected RdsUpdate doParse(XdsResourceType.Args args, Message unpackedMessage) | |||
| (RouteConfiguration) unpackedMessage, FilterRegistry.getDefaultRegistry(), args); | |||
| } | |||
|
|
|||
| private static RdsUpdate processRouteConfiguration( | |||
| static RdsUpdate processRouteConfiguration( | |||
There was a problem hiding this comment.
Why remove the private instead of calling doParse(args, routeConfiguration)?
There was a problem hiding this comment.
Good point. If you're curious, it was because type Message didn't sound like it was a data object and I didn't want to construct something just to parse it. Probably related to limit time interacting with protos.
| return builder.toString(); | ||
| } | ||
|
|
||
| public String getClusterName() { |
There was a problem hiding this comment.
Why aren't the fields private if there are getters?
There was a problem hiding this comment.
Originally didn't have getters, but you are right they should be private.
| checkNotNull(subscription, "subscription"); | ||
| String clusterName = subscription.getClusterName(); | ||
| Set<ClusterSubscription> subscriptions = clusterSubscriptions.get(clusterName); | ||
| if (subscriptions == null) { |
There was a problem hiding this comment.
|| !subscriptions.remove(subscription), since that is a common "already released" case?
| private final XdsConfigWatcher xdsConfigWatcher; | ||
| private final SynchronizationContext syncContext; | ||
| private final String dataPlaneAuthority; | ||
| private final Map<String, Set<ClusterSubscription>> clusterSubscriptions = new HashMap<>(); |
There was a problem hiding this comment.
I don't see any synchronization for mutations to this map, or requirements placed on the callers.
There was a problem hiding this comment.
Changed so that now all updates and gets happen in syncContext. Verified all uses of both clusterSubscriptions and resourceWatchers are in the syncContext
| case AGGREGATE: | ||
| for (String cluster : cdsUpdate.prioritizedClusterNames()) { | ||
| CdsWatcher clusterWatcher = | ||
| (CdsWatcher) resourceWatchers.get(CLUSTER_RESOURCE).watchers.get(cluster); |
There was a problem hiding this comment.
What if this cluster is used by another aggregate cluster or used directly by the route? I think that same sort of problem could exist for EDS. Just because it isn't any more by this cluster doesn't mean it isn't used any more.
There was a problem hiding this comment.
I thought that clusters couldn't be in multiple aggregates. I started implementing a more complicated tracking of who was using a cluster, but believed that the situations you listed weren't allowed so kept it simple.
| if (subscriptions.isEmpty()) { | ||
| clusterSubscriptions.remove(clusterName); | ||
| XdsWatcherBase<?> cdsWatcher = | ||
| resourceWatchers.get(CLUSTER_RESOURCE).watchers.get(clusterName); |
There was a problem hiding this comment.
This can only be accessed from the syncContext, right? This is not running in the syncContext.
The synchronization looks funny, especially with maybePublishConfig() entering the syncContext. It seems ever call to maybePublishConfig() would already need to be in the syncContext since that's the only way to change state.
There was a problem hiding this comment.
Removed the syncContext from inside maybePublishConfig() and do this methods logic in the syncContext.
| } | ||
|
|
||
| typeWatchers.add(resourceName, watcher); | ||
| xdsClient.watchXdsResource(type, resourceName, watcher); |
There was a problem hiding this comment.
Use the 4-arg version, to pass in the executor to use (I assume syncContext). I think we would have deprecated/deleted this 3-arg version, but it is used a lot in tests. If you enter the syncContext manually within the callback, then ADS flow control stops working properly.
…ds aggregate and eds entries.
This is just defining the XdsDependencyManager and associated classes; no movement of any functionality.