From e78d1c95efd73597eeb0e199ed4e825a95c1e182 Mon Sep 17 00:00:00 2001 From: ZHANG Dapeng Date: Wed, 29 Apr 2020 13:53:53 -0700 Subject: [PATCH] core: support default method config in service config (#6987) References: service config spec change: grpc/grpc-proto#75 c-core implementation grpc/grpc#22232 --- .../internal/ManagedChannelServiceConfig.java | 29 +++- .../internal/ServiceConfigInterceptor.java | 12 +- .../ManagedChannelServiceConfigTest.java | 84 ++++++++++ .../ServiceConfigInterceptorTest.java | 155 +++++++++++------- .../grpc/internal/ServiceConfigStateTest.java | 3 + 5 files changed, 215 insertions(+), 68 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelServiceConfig.java b/core/src/main/java/io/grpc/internal/ManagedChannelServiceConfig.java index a5d4accd02..fc78d2231a 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelServiceConfig.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelServiceConfig.java @@ -37,6 +37,8 @@ import javax.annotation.Nullable; */ final class ManagedChannelServiceConfig { + @Nullable + private final MethodInfo defaultMethodConfig; private final Map serviceMethodMap; private final Map serviceMap; @Nullable @@ -47,11 +49,13 @@ final class ManagedChannelServiceConfig { private final Map healthCheckingConfig; ManagedChannelServiceConfig( + @Nullable MethodInfo defaultMethodConfig, Map serviceMethodMap, Map serviceMap, @Nullable Throttle retryThrottling, @Nullable Object loadBalancingConfig, @Nullable Map healthCheckingConfig) { + this.defaultMethodConfig = defaultMethodConfig; this.serviceMethodMap = Collections.unmodifiableMap(new HashMap<>(serviceMethodMap)); this.serviceMap = Collections.unmodifiableMap(new HashMap<>(serviceMap)); this.retryThrottling = retryThrottling; @@ -66,6 +70,7 @@ final class ManagedChannelServiceConfig { static ManagedChannelServiceConfig empty() { return new ManagedChannelServiceConfig( + null, new HashMap(), new HashMap(), /* retryThrottling= */ null, @@ -100,6 +105,7 @@ final class ManagedChannelServiceConfig { // this is surprising, but possible. return new ManagedChannelServiceConfig( + null, serviceMethodMap, serviceMap, retryThrottling, @@ -107,6 +113,7 @@ final class ManagedChannelServiceConfig { healthCheckingConfig); } + MethodInfo defaultMethodConfig = null; for (Map methodConfig : methodConfigs) { MethodInfo info = new MethodInfo( methodConfig, retryEnabled, maxRetryAttemptsLimit, maxHedgedAttemptsLimit); @@ -114,13 +121,21 @@ final class ManagedChannelServiceConfig { List> nameList = ServiceConfigUtil.getNameListFromMethodConfig(methodConfig); - checkArgument( - nameList != null && !nameList.isEmpty(), "no names in method config %s", methodConfig); + if (nameList == null || nameList.isEmpty()) { + continue; + } for (Map name : nameList) { String serviceName = ServiceConfigUtil.getServiceFromName(name); - checkArgument(!Strings.isNullOrEmpty(serviceName), "missing service name"); String methodName = ServiceConfigUtil.getMethodFromName(name); - if (Strings.isNullOrEmpty(methodName)) { + if (Strings.isNullOrEmpty(serviceName)) { + checkArgument( + Strings.isNullOrEmpty(methodName), "missing service name for method %s", methodName); + checkArgument( + defaultMethodConfig == null, + "Duplicate default method config in service config %s", + serviceConfig); + defaultMethodConfig = info; + } else if (Strings.isNullOrEmpty(methodName)) { // Service scoped config checkArgument( !serviceMap.containsKey(serviceName), "Duplicate service %s", serviceName); @@ -139,6 +154,7 @@ final class ManagedChannelServiceConfig { return new ManagedChannelServiceConfig( + defaultMethodConfig, serviceMethodMap, serviceMap, retryThrottling, @@ -165,6 +181,11 @@ final class ManagedChannelServiceConfig { return serviceMethodMap; } + @Nullable + MethodInfo getDefaultMethodConfig() { + return defaultMethodConfig; + } + @VisibleForTesting @Nullable Object getLoadBalancingConfig() { diff --git a/core/src/main/java/io/grpc/internal/ServiceConfigInterceptor.java b/core/src/main/java/io/grpc/internal/ServiceConfigInterceptor.java index f27f9efa78..9fb91db5ea 100644 --- a/core/src/main/java/io/grpc/internal/ServiceConfigInterceptor.java +++ b/core/src/main/java/io/grpc/internal/ServiceConfigInterceptor.java @@ -174,14 +174,18 @@ final class ServiceConfigInterceptor implements ClientInterceptor { @CheckForNull private MethodInfo getMethodInfo(MethodDescriptor method) { ManagedChannelServiceConfig mcsc = managedChannelServiceConfig.get(); - MethodInfo info = null; - if (mcsc != null) { - info = mcsc.getServiceMethodMap().get(method.getFullMethodName()); + if (mcsc == null) { + return null; } - if (info == null && mcsc != null) { + MethodInfo info; + info = mcsc.getServiceMethodMap().get(method.getFullMethodName()); + if (info == null) { String serviceName = method.getServiceName(); info = mcsc.getServiceMap().get(serviceName); } + if (info == null) { + info = mcsc.getDefaultMethodConfig(); + } return info; } diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelServiceConfigTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelServiceConfigTest.java index ad6c73de8f..5a7bad5a1b 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelServiceConfigTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelServiceConfigTest.java @@ -18,15 +18,22 @@ package io.grpc.internal; import static com.google.common.truth.Truth.assertThat; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import java.util.Collections; import java.util.Map; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @RunWith(JUnit4.class) public class ManagedChannelServiceConfigTest { + @Rule + public final ExpectedException thrown = ExpectedException.none(); + @Test public void managedChannelServiceConfig_shouldParseHealthCheckingConfig() throws Exception { Map rawServiceConfig = @@ -52,6 +59,83 @@ public class ManagedChannelServiceConfigTest { assertThat(mcsc.getHealthCheckingConfig()).isNull(); } + @Test + public void createManagedChannelServiceConfig_failsOnDuplicateMethod() { + Map name1 = ImmutableMap.of("service", "service", "method", "method"); + Map name2 = ImmutableMap.of("service", "service", "method", "method"); + Map methodConfig = ImmutableMap.of("name", ImmutableList.of(name1, name2)); + Map serviceConfig = ImmutableMap.of("methodConfig", ImmutableList.of(methodConfig)); + + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("Duplicate method"); + + ManagedChannelServiceConfig.fromServiceConfig(serviceConfig, true, 3, 4, null); + } + + @Test + public void createManagedChannelServiceConfig_failsOnDuplicateService() { + Map name1 = ImmutableMap.of("service", "service"); + Map name2 = ImmutableMap.of("service", "service"); + Map methodConfig = ImmutableMap.of("name", ImmutableList.of(name1, name2)); + Map serviceConfig = ImmutableMap.of("methodConfig", ImmutableList.of(methodConfig)); + + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("Duplicate service"); + + ManagedChannelServiceConfig.fromServiceConfig(serviceConfig, true, 3, 4, null); + } + + @Test + public void createManagedChannelServiceConfig_failsOnDuplicateServiceMultipleConfig() { + Map name1 = ImmutableMap.of("service", "service"); + Map name2 = ImmutableMap.of("service", "service"); + Map methodConfig1 = ImmutableMap.of("name", ImmutableList.of(name1)); + Map methodConfig2 = ImmutableMap.of("name", ImmutableList.of(name2)); + Map serviceConfig = + ImmutableMap.of("methodConfig", ImmutableList.of(methodConfig1, methodConfig2)); + + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("Duplicate service"); + + ManagedChannelServiceConfig.fromServiceConfig(serviceConfig, true, 3, 4, null); + } + + @Test + public void createManagedChannelServiceConfig_failsOnMethodNameWithEmptyServiceName() { + Map name = ImmutableMap.of("service", "", "method", "method1"); + Map methodConfig = ImmutableMap.of("name", ImmutableList.of(name)); + Map serviceConfig = ImmutableMap.of("methodConfig", ImmutableList.of(methodConfig)); + + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("missing service name for method method1"); + + ManagedChannelServiceConfig.fromServiceConfig(serviceConfig, true, 3, 4, null); + } + + @Test + public void createManagedChannelServiceConfig_failsOnMethodNameWithoutServiceName() { + Map name = ImmutableMap.of("method", "method1"); + Map methodConfig = ImmutableMap.of("name", ImmutableList.of(name)); + Map serviceConfig = ImmutableMap.of("methodConfig", ImmutableList.of(methodConfig)); + + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("missing service name for method method1"); + + ManagedChannelServiceConfig.fromServiceConfig(serviceConfig, true, 3, 4, null); + } + + @Test + public void createManagedChannelServiceConfig_failsOnMissingServiceName() { + Map name = ImmutableMap.of("method", "method"); + Map methodConfig = ImmutableMap.of("name", ImmutableList.of(name)); + Map serviceConfig = ImmutableMap.of("methodConfig", ImmutableList.of(methodConfig)); + + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("missing service"); + + ManagedChannelServiceConfig.fromServiceConfig(serviceConfig, true, 3, 4, null); + } + @SuppressWarnings("unchecked") private static Map parseConfig(String json) throws Exception { return (Map) JsonParser.parse(json); diff --git a/core/src/test/java/io/grpc/internal/ServiceConfigInterceptorTest.java b/core/src/test/java/io/grpc/internal/ServiceConfigInterceptorTest.java index d339e4423d..eaa6748031 100644 --- a/core/src/test/java/io/grpc/internal/ServiceConfigInterceptorTest.java +++ b/core/src/test/java/io/grpc/internal/ServiceConfigInterceptorTest.java @@ -304,81 +304,61 @@ public class ServiceConfigInterceptorTest { } @Test - public void handleUpdate_failsOnMissingServiceName() { - JsonObj name = new JsonObj("method", "method"); - JsonObj methodConfig = new JsonObj("name", new JsonList(name)); - JsonObj serviceConfig = new JsonObj("methodConfig", new JsonList(methodConfig)); - - thrown.expect(IllegalArgumentException.class); - thrown.expectMessage("missing service"); - - ManagedChannelServiceConfig parsedServiceConfig = - createManagedChannelServiceConfig(serviceConfig); - - interceptor.handleUpdate(parsedServiceConfig); - } - - @Test - public void handleUpdate_failsOnDuplicateMethod() { - JsonObj name1 = new JsonObj("service", "service", "method", "method"); - JsonObj name2 = new JsonObj("service", "service", "method", "method"); - JsonObj methodConfig = new JsonObj("name", new JsonList(name1, name2)); - JsonObj serviceConfig = new JsonObj("methodConfig", new JsonList(methodConfig)); - - thrown.expect(IllegalArgumentException.class); - thrown.expectMessage("Duplicate method"); - - ManagedChannelServiceConfig parsedServiceConfig = - createManagedChannelServiceConfig(serviceConfig); - - interceptor.handleUpdate(parsedServiceConfig); - } - - @Test - public void handleUpdate_failsOnEmptyName() { + public void handleUpdate_onEmptyName() { JsonObj methodConfig = new JsonObj(); JsonObj serviceConfig = new JsonObj("methodConfig", new JsonList(methodConfig)); - - thrown.expect(IllegalArgumentException.class); - thrown.expectMessage("no names in method config"); - ManagedChannelServiceConfig parsedServiceConfig = createManagedChannelServiceConfig(serviceConfig); interceptor.handleUpdate(parsedServiceConfig); + + assertThat(interceptor.managedChannelServiceConfig.get().getDefaultMethodConfig()).isNull(); + assertThat(interceptor.managedChannelServiceConfig.get().getServiceMap()).isEmpty(); + assertThat(interceptor.managedChannelServiceConfig.get().getServiceMethodMap()).isEmpty(); } @Test - public void handleUpdate_failsOnDuplicateService() { - JsonObj name1 = new JsonObj("service", "service"); - JsonObj name2 = new JsonObj("service", "service"); - JsonObj methodConfig = new JsonObj("name", new JsonList(name1, name2)); + public void handleUpdate_onDefaultMethodConfig() { + JsonObj name = new JsonObj(); + JsonObj methodConfig = new JsonObj("name", new JsonList(name)); JsonObj serviceConfig = new JsonObj("methodConfig", new JsonList(methodConfig)); - - thrown.expect(IllegalArgumentException.class); - thrown.expectMessage("Duplicate service"); - ManagedChannelServiceConfig parsedServiceConfig = createManagedChannelServiceConfig(serviceConfig); - interceptor.handleUpdate(parsedServiceConfig); - } - - @Test - public void handleUpdate_failsOnDuplicateServiceMultipleConfig() { - JsonObj name1 = new JsonObj("service", "service"); - JsonObj name2 = new JsonObj("service", "service"); - JsonObj methodConfig1 = new JsonObj("name", new JsonList(name1)); - JsonObj methodConfig2 = new JsonObj("name", new JsonList(name2)); - JsonObj serviceConfig = new JsonObj("methodConfig", new JsonList(methodConfig1, methodConfig2)); - - thrown.expect(IllegalArgumentException.class); - thrown.expectMessage("Duplicate service"); - - ManagedChannelServiceConfig parsedServiceConfig = - createManagedChannelServiceConfig(serviceConfig); + assertThat(interceptor.managedChannelServiceConfig.get().getDefaultMethodConfig()) + .isEqualTo(new MethodInfo(methodConfig, false, 1, 1)); + assertThat(interceptor.managedChannelServiceConfig.get().getServiceMap()).isEmpty(); + assertThat(interceptor.managedChannelServiceConfig.get().getServiceMethodMap()).isEmpty(); + name = new JsonObj("method", ""); + methodConfig = new JsonObj("name", new JsonList(name)); + serviceConfig = new JsonObj("methodConfig", new JsonList(methodConfig)); + parsedServiceConfig = createManagedChannelServiceConfig(serviceConfig); interceptor.handleUpdate(parsedServiceConfig); + assertThat(interceptor.managedChannelServiceConfig.get().getDefaultMethodConfig()) + .isEqualTo(new MethodInfo(methodConfig, false, 1, 1)); + assertThat(interceptor.managedChannelServiceConfig.get().getServiceMap()).isEmpty(); + assertThat(interceptor.managedChannelServiceConfig.get().getServiceMethodMap()).isEmpty(); + + name = new JsonObj("service", ""); + methodConfig = new JsonObj("name", new JsonList(name)); + serviceConfig = new JsonObj("methodConfig", new JsonList(methodConfig)); + parsedServiceConfig = createManagedChannelServiceConfig(serviceConfig); + interceptor.handleUpdate(parsedServiceConfig); + assertThat(interceptor.managedChannelServiceConfig.get().getDefaultMethodConfig()) + .isEqualTo(new MethodInfo(methodConfig, false, 1, 1)); + assertThat(interceptor.managedChannelServiceConfig.get().getServiceMap()).isEmpty(); + assertThat(interceptor.managedChannelServiceConfig.get().getServiceMethodMap()).isEmpty(); + + name = new JsonObj("service", "", "method", ""); + methodConfig = new JsonObj("name", new JsonList(name)); + serviceConfig = new JsonObj("methodConfig", new JsonList(methodConfig)); + parsedServiceConfig = createManagedChannelServiceConfig(serviceConfig); + interceptor.handleUpdate(parsedServiceConfig); + assertThat(interceptor.managedChannelServiceConfig.get().getDefaultMethodConfig()) + .isEqualTo(new MethodInfo(methodConfig, false, 1, 1)); + assertThat(interceptor.managedChannelServiceConfig.get().getServiceMap()).isEmpty(); + assertThat(interceptor.managedChannelServiceConfig.get().getServiceMethodMap()).isEmpty(); } @Test @@ -425,6 +405,61 @@ public class ServiceConfigInterceptorTest { "service2", new MethodInfo(methodConfig, false, 1, 1)); } + @Test + public void interceptCall_matchNames() { + JsonObj name0 = new JsonObj(); + JsonObj name1 = new JsonObj("service", "service"); + JsonObj name2 = new JsonObj("service", "service", "method", "method"); + JsonObj methodConfig0 = new JsonObj( + "name", new JsonList(name0), "maxRequestMessageBytes", 5d); + JsonObj methodConfig1 = new JsonObj( + "name", new JsonList(name1), "maxRequestMessageBytes", 6d); + JsonObj methodConfig2 = new JsonObj( + "name", new JsonList(name2), "maxRequestMessageBytes", 7d); + JsonObj serviceConfig = + new JsonObj("methodConfig", new JsonList(methodConfig0, methodConfig1, methodConfig2)); + ManagedChannelServiceConfig parsedServiceConfig = + createManagedChannelServiceConfig(serviceConfig); + + interceptor.handleUpdate(parsedServiceConfig); + + String fullMethodName = + MethodDescriptor.generateFullMethodName("service", "method"); + MethodDescriptor methodDescriptor = + MethodDescriptor.newBuilder(new NoopMarshaller(), new NoopMarshaller()) + .setType(MethodType.UNARY) + .setFullMethodName(fullMethodName) + .build(); + interceptor.interceptCall( + methodDescriptor, CallOptions.DEFAULT, channel); + verify(channel).newCall(eq(methodDescriptor), callOptionsCap.capture()); + assertThat(callOptionsCap.getValue().getMaxOutboundMessageSize()).isEqualTo(7); + + fullMethodName = + MethodDescriptor.generateFullMethodName("service", "method2"); + methodDescriptor = + MethodDescriptor.newBuilder(new NoopMarshaller(), new NoopMarshaller()) + .setType(MethodType.UNARY) + .setFullMethodName(fullMethodName) + .build(); + interceptor.interceptCall( + methodDescriptor, CallOptions.DEFAULT, channel); + verify(channel).newCall(eq(methodDescriptor), callOptionsCap.capture()); + assertThat(callOptionsCap.getValue().getMaxOutboundMessageSize()).isEqualTo(6); + + fullMethodName = + MethodDescriptor.generateFullMethodName("service2", "method"); + methodDescriptor = + MethodDescriptor.newBuilder(new NoopMarshaller(), new NoopMarshaller()) + .setType(MethodType.UNARY) + .setFullMethodName(fullMethodName) + .build(); + interceptor.interceptCall( + methodDescriptor, CallOptions.DEFAULT, channel); + verify(channel).newCall(eq(methodDescriptor), callOptionsCap.capture()); + assertThat(callOptionsCap.getValue().getMaxOutboundMessageSize()).isEqualTo(5); + } + @Test public void methodInfo_validateDeadline() { JsonObj name = new JsonObj("service", "service"); diff --git a/core/src/test/java/io/grpc/internal/ServiceConfigStateTest.java b/core/src/test/java/io/grpc/internal/ServiceConfigStateTest.java index 868fb15b47..4390ff44ea 100644 --- a/core/src/test/java/io/grpc/internal/ServiceConfigStateTest.java +++ b/core/src/test/java/io/grpc/internal/ServiceConfigStateTest.java @@ -39,12 +39,14 @@ import org.junit.runners.JUnit4; public class ServiceConfigStateTest { private final ManagedChannelServiceConfig serviceConfig1 = new ManagedChannelServiceConfig( + null, Collections.emptyMap(), Collections.emptyMap(), null, null, null); private final ManagedChannelServiceConfig serviceConfig2 = new ManagedChannelServiceConfig( + null, Collections.emptyMap(), Collections.emptyMap(), null, @@ -428,6 +430,7 @@ public class ServiceConfigStateTest { public void lookup_default_onPresent_onPresent() { ServiceConfigState scs = new ServiceConfigState(serviceConfig1, true); ManagedChannelServiceConfig serviceConfig3 = new ManagedChannelServiceConfig( + null, Collections.emptyMap(), Collections.emptyMap(), null,