diff --git a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java index 328995d22da..7fa1d75c71c 100644 --- a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java +++ b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java @@ -476,21 +476,7 @@ public Result selectConfig(PickSubchannelArgs args) { } while (!retainCluster(cluster)); final RouteAction routeAction = selectedRoute.routeAction; - Long timeoutNanos = null; - if (enableTimeout) { - timeoutNanos = routeAction.timeoutNano(); - if (timeoutNanos == null) { - timeoutNanos = routingCfg.fallbackTimeoutNano; - } - if (timeoutNanos <= 0) { - timeoutNanos = null; - } - } - RetryPolicy retryPolicy = routeAction.retryPolicy(); - // TODO(chengyuanzhang): avoid service config generation and parsing for each call. - Map rawServiceConfig = - generateServiceConfigWithMethodConfig(timeoutNanos, retryPolicy); - ConfigOrError parsedServiceConfig = serviceConfigParser.parseServiceConfig(rawServiceConfig); + ConfigOrError parsedServiceConfig = selectedRoute.parsedServiceConfig; Object config = parsedServiceConfig.getConfig(); if (config == null) { releaseCluster(cluster); @@ -781,15 +767,31 @@ private void updateRoutes( Map rlsPluginConfigMap = new HashMap<>(); for (Route route : routes) { RouteAction action = route.routeAction(); + ConfigOrError parsedServiceConfig = null; + if (action != null) { + Long timeoutNanos = null; + if (enableTimeout) { + timeoutNanos = action.timeoutNano(); + if (timeoutNanos == null) { + timeoutNanos = httpMaxStreamDurationNano; + } + if (timeoutNanos <= 0) { + timeoutNanos = null; + } + } + // These settings depend only on the routing snapshot, not on the individual RPC. + parsedServiceConfig = serviceConfigParser.parseServiceConfig( + generateServiceConfigWithMethodConfig(timeoutNanos, action.retryPolicy())); + } String prefixedName; if (action == null) { - routesData.add(new RouteData(route.routeMatch(), null, ImmutableList.of())); + routesData.add(new RouteData(route.routeMatch(), null, ImmutableList.of(), null)); } else if (action.cluster() != null) { prefixedName = prefixedClusterName(action.cluster()); clusters.add(prefixedName); clusterNameMap.put(prefixedName, action.cluster()); ClientInterceptor filters = createFilters(filterConfigs, virtualHost, route, null); - routesData.add(new RouteData(route.routeMatch(), route.routeAction(), filters)); + routesData.add(new RouteData(route.routeMatch(), action, filters, parsedServiceConfig)); } else if (action.weightedClusters() != null) { ImmutableList.Builder filterList = ImmutableList.builder(); for (ClusterWeight weightedCluster : action.weightedClusters()) { @@ -799,7 +801,7 @@ private void updateRoutes( filterList.add(createFilters(filterConfigs, virtualHost, route, weightedCluster)); } routesData.add( - new RouteData(route.routeMatch(), route.routeAction(), filterList.build())); + new RouteData(route.routeMatch(), action, filterList.build(), parsedServiceConfig)); } else if (action.namedClusterSpecifierPluginConfig() != null) { PluginConfig pluginConfig = action.namedClusterSpecifierPluginConfig().config(); if (pluginConfig instanceof RlsPluginConfig) { @@ -809,7 +811,7 @@ private void updateRoutes( rlsPluginConfigMap.put(prefixedName, (RlsPluginConfig) pluginConfig); } ClientInterceptor filters = createFilters(filterConfigs, virtualHost, route, null); - routesData.add(new RouteData(route.routeMatch(), route.routeAction(), filters)); + routesData.add(new RouteData(route.routeMatch(), action, filters, parsedServiceConfig)); } else { // Discard route } @@ -864,7 +866,7 @@ private void updateRoutes( } // Make newly added clusters selectable by config selector and deleted clusters no longer // selectable. - routingConfig = new RoutingConfig(xdsConfig, httpMaxStreamDurationNano, routesData.build()); + routingConfig = new RoutingConfig(xdsConfig, routesData.build()); for (String cluster : deletedClusters) { int count = clusterRefs.get(cluster).refCount.decrementAndGet(); if (count == 0) { @@ -955,21 +957,18 @@ private void cleanUpRoutes(Status error) { */ private static class RoutingConfig { final XdsConfig xdsConfig; - final long fallbackTimeoutNano; final ImmutableList routes; final Status errorStatus; private RoutingConfig( - XdsConfig xdsConfig, long fallbackTimeoutNano, ImmutableList routes) { + XdsConfig xdsConfig, ImmutableList routes) { this.xdsConfig = checkNotNull(xdsConfig, "xdsConfig"); - this.fallbackTimeoutNano = fallbackTimeoutNano; this.routes = checkNotNull(routes, "routes"); this.errorStatus = null; } private RoutingConfig(Status errorStatus) { this.xdsConfig = null; - this.fallbackTimeoutNano = 0; this.routes = null; this.errorStatus = checkNotNull(errorStatus, "errorStatus"); checkArgument(!errorStatus.isOk(), "errorStatus should not be okay"); @@ -987,20 +986,27 @@ static final class RouteData { * clusters. */ final ImmutableList filterChoices; + /** Parsed method config shared by RPCs using this route; null for non-forwarding actions. */ + @Nullable + final ConfigOrError parsedServiceConfig; - RouteData(RouteMatch routeMatch, @Nullable RouteAction routeAction, ClientInterceptor filter) { - this(routeMatch, routeAction, ImmutableList.of(filter)); + RouteData(RouteMatch routeMatch, @Nullable RouteAction routeAction, ClientInterceptor filter, + @Nullable ConfigOrError parsedServiceConfig) { + this(routeMatch, routeAction, ImmutableList.of(filter), parsedServiceConfig); } RouteData( RouteMatch routeMatch, @Nullable RouteAction routeAction, - ImmutableList filterChoices) { + ImmutableList filterChoices, + @Nullable ConfigOrError parsedServiceConfig) { this.routeMatch = checkNotNull(routeMatch, "routeMatch"); checkArgument( routeAction == null || !filterChoices.isEmpty(), "filter may be empty only for non-forwarding action"); this.routeAction = routeAction; + this.parsedServiceConfig = routeAction == null ? null + : checkNotNull(parsedServiceConfig, "parsedServiceConfig"); if (routeAction != null && routeAction.weightedClusters() != null) { checkArgument( routeAction.weightedClusters().size() == filterChoices.size(), diff --git a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java b/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java index e57d0860588..eb307483720 100644 --- a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java @@ -822,6 +822,18 @@ public void resolved_simpleCallSucceeds() { verifyNoMoreInteractions(mockListener); } + @Test + public void resolved_configReused() { + InternalConfigSelector selector = resolveToClusters(); + Result first = selector.selectConfig( + newPickSubchannelArgs(call1.methodDescriptor, new Metadata(), CallOptions.DEFAULT)); + Result second = selector.selectConfig( + newPickSubchannelArgs(call1.methodDescriptor, new Metadata(), CallOptions.DEFAULT)); + assertThat(first.getStatus().isOk()).isTrue(); + assertThat(second.getStatus().isOk()).isTrue(); + assertThat(second.getConfig()).isSameInstanceAs(first.getConfig()); + } + @Test public void resolved_simpleCallFailedToRoute_noMatchingRoute() { InternalConfigSelector configSelector = resolveToClusters();