diff --git a/java-bigtable/google-cloud-bigtable/pom.xml b/java-bigtable/google-cloud-bigtable/pom.xml index ff6a421d6ed4..e08d055d7de5 100644 --- a/java-bigtable/google-cloud-bigtable/pom.xml +++ b/java-bigtable/google-cloud-bigtable/pom.xml @@ -606,6 +606,7 @@ ${bigtable.cfe-jwt-audience} ${bigtable.enable-grpc-logs} ${project.build.directory}/test-grpc-logs/session-it + true session_configuration { session_diversion_configuration { global_diversion { session_load: 1.0 } } } diff --git a/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/util/ClientConfigurationManager.java b/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/util/ClientConfigurationManager.java index baf9ea2920fb..5d1569b39e5d 100644 --- a/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/util/ClientConfigurationManager.java +++ b/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/util/ClientConfigurationManager.java @@ -67,14 +67,45 @@ import javax.annotation.concurrent.GuardedBy; /** - * ClientConfiguration controls how the data client should behave. For example, it controls how much - * of the traffic should go through jetstream vs unary. It also controls how frequent the client - * should poll for a new configuration and how long the configuration is valid for. + * Manages the {@link ClientConfiguration} that controls how the data client behaves (for example, + * how much traffic is routed through sessions vs unary RPCs, how frequently to poll for a new + * configuration, and how long a configuration remains valid). + * + *

Configuration Resolution: + * + *

*/ public class ClientConfigurationManager implements AutoCloseable { private static final Logger logger = Logger.getLogger(ClientConfigurationManager.class.getName()); + /** + * System property providing a textproto {@link ClientConfiguration} to use locally. When set, the + * server configuration is ignored. Used as-is by default, or merged on top of {@link + * #loadDefault()} when {@link #OVERRIDE_USE_DEFAULT_SYS_PROP_KEY} is {@code true}. + */ public static final String OVERRIDE_SYS_PROP_KEY = "bigtable.internal.client-config-override"; + + /** + * Boolean system property for ergonomics in testing and probing that, when {@code true}, merges + * {@link #OVERRIDE_SYS_PROP_KEY} on top of {@link #loadDefault()} so callers only need to specify + * the fields they care about rather than the full configuration. Ignored when {@link + * #OVERRIDE_SYS_PROP_KEY} is not set. + */ + public static final String OVERRIDE_USE_DEFAULT_SYS_PROP_KEY = + "bigtable.internal.client-config-override-use-default"; + public static final String DISABLE_DIRECT_ACCESS_FALLBACK_ENV_VAR = "CBT_DISABLE_DIRECT_ACCESS_FALLBACK"; public static final String DISABLE_DIRECT_ACCESS_FALLBACK_SYS_PROP_KEY = @@ -125,6 +156,7 @@ public void close() { private final ChannelProvider channelProvider; @GuardedBy("this") + @Nullable private ManagedChannel channel; private final DebugTagTracer debugTagTracer; @@ -173,12 +205,18 @@ public ClientConfigurationManager( ScheduledExecutorService executor) throws IOException { this.defaultConfig = loadDefault(); + boolean overrideUseDefault = + Boolean.parseBoolean(sysProps.getProperty(OVERRIDE_USE_DEFAULT_SYS_PROP_KEY)); this.overrideConfig = Optional.ofNullable(sysProps.getProperty(OVERRIDE_SYS_PROP_KEY)) .map( str -> { try { - return TextFormat.parse(str, ClientConfiguration.class); + ClientConfiguration parsed = TextFormat.parse(str, ClientConfiguration.class); + if (overrideUseDefault) { + return defaultConfig.toBuilder().mergeFrom(parsed).build(); + } + return parsed; } catch (Exception e) { throw new RuntimeException( "Failed to parse bigtable.internal.client-config-override", e); @@ -200,7 +238,7 @@ public ClientConfigurationManager( .printToString(overrideConfig.get())); } featureFlags = channelProvider.updateFeatureFlags(featureFlags); - this.clientConfiguration = normalizeConfig(defaultConfig); + this.clientConfiguration = normalizeConfig(overrideConfig.orElse(defaultConfig)); this.metadata = Util.composeMetadata( @@ -215,7 +253,8 @@ public ClientConfigurationManager( .build(); this.channelProvider = channelProvider; - this.channel = channelProvider.newChannelBuilder().build(); + this.channel = + overrideConfig.isPresent() ? null : channelProvider.newChannelBuilder().build(); this.debugTagTracer = tracer; this.executor = executor; } @@ -239,6 +278,11 @@ public static ClientConfiguration loadDefault() throws IOException { * back to unary if it fails. */ public CompletableFuture start() { + if (overrideConfig.isPresent()) { + ClientConfiguration cfg = getClientConfiguration(); + logger.log(Level.CONFIG, "Using local override config: {0}", cfg); + return CompletableFuture.completedFuture(cfg); + } CompletableFuture f = startPolling(); @SuppressWarnings("UnusedVariable") CompletableFuture ignored = @@ -261,7 +305,9 @@ public synchronized void close() { if (nextPoll != null) { nextPoll.cancel(true); } - channel.shutdown(); + if (channel != null) { + channel.shutdown(); + } } /** Gets the client configuration. */ @@ -398,9 +444,6 @@ private ClientConfiguration normalizeConfig(ClientConfiguration cfg) { .setDebugTagLevel(defaultConfig.getTelemetryConfiguration().getDebugTagLevel()); } - // Inject overrides - overrideConfig.ifPresent(builder::mergeFrom); - if (disableDirectPathFallback && builder .getSessionConfiguration() @@ -413,9 +456,7 @@ private ClientConfiguration normalizeConfig(ClientConfiguration cfg) { .setDirectAccessOnly(DirectAccessOnly.getDefaultInstance()); } - // When sessions are disabled make sure to clear out the config. Read from the builder, not - // cfg, so that a diversion config supplied via the override sys-prop is honoured even when - // the server-returned config disables sessions. + // When sessions are disabled make sure to clear out the config. if (!divertsAnyTrafficToSessions(builder.getSessionConfiguration())) { builder.clearSessionConfiguration(); return builder.build(); diff --git a/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/util/ClientConfigurationManagerTest.java b/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/util/ClientConfigurationManagerTest.java index 3c95df08facd..91c0f422c5e5 100644 --- a/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/util/ClientConfigurationManagerTest.java +++ b/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/util/ClientConfigurationManagerTest.java @@ -251,78 +251,66 @@ void testDisabledSessions() throws ExecutionException, InterruptedException { } @Test - void testDisabledSessionsOverriddenBySysProp() - throws ExecutionException, InterruptedException, IOException { - // Server returns session_load=0 (sessions disabled from the server's perspective)… - ClientConfiguration.Builder disabledBuilder = manager.getDefaultConfig().toBuilder(); - disabledBuilder.getSessionConfigurationBuilder().setSessionLoad(0); - service.config.set(disabledBuilder.build()); - - // …but a sys-prop override forces session_load>0 to opt this client into sessions. + void testSysPropOverride() throws Exception { manager.close(); - String clientConfigOverrides = - TextFormat.printer() - .printToString( - ClientConfiguration.newBuilder() - .setSessionConfiguration( - SessionClientConfiguration.newBuilder() - .setSessionDiversionConfiguration( - SessionDiversionConfiguration.newBuilder() - .setGlobalDiversion( - SessionScopeDiversionConfiguration.newBuilder() - .setSessionLoad(0.75f)))) - .build()); - Properties sysProps = new Properties(); - sysProps.setProperty(ClientConfigurationManager.OVERRIDE_SYS_PROP_KEY, clientConfigOverrides); + + ClientConfiguration rawOverrideProto = + ClientConfiguration.newBuilder() + .setSessionConfiguration( + SessionClientConfiguration.newBuilder() + .setSessionDiversionConfiguration( + SessionDiversionConfiguration.newBuilder() + .setGlobalDiversion( + SessionScopeDiversionConfiguration.newBuilder() + .setSessionLoad(0.75f))) + .setSessionPoolConfiguration( + SessionPoolConfiguration.newBuilder().setHeadroom(0.7f))) + .build(); + String clientConfigOverrides = TextFormat.printer().printToString(rawOverrideProto); + + // 1. Raw override without OVERRIDE_USE_DEFAULT_SYS_PROP_KEY: completely ignores both server + // config and defaultConfig, uses only the textproto, and does not poll the server. + Properties rawSysProps = new Properties(); + rawSysProps.setProperty( + ClientConfigurationManager.OVERRIDE_SYS_PROP_KEY, clientConfigOverrides); manager = new ClientConfigurationManager( - sysProps, FEATURE_FLAGS, CLIENT_INFO, channelProvider, noopDebugTracer, mockExecutor); - - ClientConfiguration resolvedConfig = manager.start().get(); - - // The override must be honoured: the diversion config and the default SessionPoolConfiguration - // must survive normalization instead of being cleared as if sessions were disabled. - assertThat( - resolvedConfig - .getSessionConfiguration() - .getSessionDiversionConfiguration() - .getGlobalDiversion() - .getSessionLoad()) - .isEqualTo(0.75f); - assertThat(resolvedConfig.getSessionConfiguration().getSessionPoolConfiguration()) - .isEqualTo( - manager.getDefaultConfig().getSessionConfiguration().getSessionPoolConfiguration()); + rawSysProps, + FEATURE_FLAGS, + CLIENT_INFO, + channelProvider, + noopDebugTracer, + mockExecutor); + + ClientConfiguration rawResolvedConfig = manager.start().get(); + ClientConfiguration expectedRawConfig = + rawOverrideProto.toBuilder() + .setTelemetryConfiguration(manager.getDefaultConfig().getTelemetryConfiguration()) + .build(); + assertThat(rawResolvedConfig).isEqualTo(expectedRawConfig); assertThat(manager.areSessionsRequired()).isTrue(); - } - - @Test - void testSysPropOverride() throws Exception { + verify(mockExecutor, never()).schedule(any(Runnable.class), anyLong(), any(TimeUnit.class)); manager.close(); - String clientConfigOverrides = - TextFormat.printer() - .printToString( - ClientConfiguration.newBuilder() - .setSessionConfiguration( - SessionClientConfiguration.newBuilder() - .setSessionDiversionConfiguration( - SessionDiversionConfiguration.newBuilder() - .setGlobalDiversion( - SessionScopeDiversionConfiguration.newBuilder() - .setSessionLoad(0.75f))) - .setSessionPoolConfiguration( - SessionPoolConfiguration.newBuilder().setHeadroom(0.7f))) - .build()); - Properties sysProps = new Properties(); - sysProps.setProperty(ClientConfigurationManager.OVERRIDE_SYS_PROP_KEY, clientConfigOverrides); + // 2. Override with OVERRIDE_USE_DEFAULT_SYS_PROP_KEY=true: merges override onto defaultConfig + // and does not poll the server. + Properties useDefaultSysProps = new Properties(); + useDefaultSysProps.setProperty( + ClientConfigurationManager.OVERRIDE_USE_DEFAULT_SYS_PROP_KEY, "true"); + useDefaultSysProps.setProperty( + ClientConfigurationManager.OVERRIDE_SYS_PROP_KEY, clientConfigOverrides); manager = new ClientConfigurationManager( - sysProps, FEATURE_FLAGS, CLIENT_INFO, channelProvider, noopDebugTracer, mockExecutor); + useDefaultSysProps, + FEATURE_FLAGS, + CLIENT_INFO, + channelProvider, + noopDebugTracer, + mockExecutor); - ClientConfiguration initialConfig = manager.start().get(); + ClientConfiguration mergedConfig = manager.start().get(); - // The effective config should be the server config with the overlayed fields - ClientConfiguration.Builder expectedConfig = service.config.get().toBuilder(); + ClientConfiguration.Builder expectedConfig = manager.getDefaultConfig().toBuilder(); expectedConfig .getSessionConfigurationBuilder() .getSessionDiversionConfigurationBuilder() @@ -333,35 +321,29 @@ void testSysPropOverride() throws Exception { .getSessionPoolConfigurationBuilder() .setHeadroom(0.7f); - assertThat(initialConfig).isEqualTo(expectedConfig.build()); + assertThat(mergedConfig).isEqualTo(expectedConfig.build()); assertThat(manager.areSessionsRequired()).isTrue(); - } - - /** Verify that proto merging works when the override the same as the default value */ - @Test - void testSysPropOverrideSessionLoadZero() throws Exception { + verify(mockExecutor, never()).schedule(any(Runnable.class), anyLong(), any(TimeUnit.class)); manager.close(); - String clientConfigOverrides = - TextFormat.printer() - .printToString( - ClientConfiguration.newBuilder() - .setSessionConfiguration( - SessionClientConfiguration.newBuilder().setSessionLoad(0)) - .build()); - Properties sysProps = new Properties(); - sysProps.setProperty(ClientConfigurationManager.OVERRIDE_SYS_PROP_KEY, clientConfigOverrides); + // 3. OVERRIDE_USE_DEFAULT_SYS_PROP_KEY=true without OVERRIDE_SYS_PROP_KEY: ignored, polls the + // server normally. + Properties onlyUseDefaultSysProps = new Properties(); + onlyUseDefaultSysProps.setProperty( + ClientConfigurationManager.OVERRIDE_USE_DEFAULT_SYS_PROP_KEY, "true"); manager = new ClientConfigurationManager( - sysProps, FEATURE_FLAGS, CLIENT_INFO, channelProvider, noopDebugTracer, mockExecutor); - - ClientConfiguration initialConfig = manager.start().get(); - - // The effective config should be the server config with the overlayed fields - ClientConfiguration.Builder expectedConfig = service.config.get().toBuilder(); - expectedConfig.getSessionConfigurationBuilder().setSessionLoad(0); - - assertThat(initialConfig).isEqualTo(service.config.get()); + onlyUseDefaultSysProps, + FEATURE_FLAGS, + CLIENT_INFO, + channelProvider, + noopDebugTracer, + mockExecutor); + + ClientConfiguration serverConfig = manager.start().get(); + assertThat(serverConfig).isEqualTo(service.config.get()); + assertThat(manager.areSessionsRequired()).isFalse(); + verify(mockExecutor, times(1)).schedule(any(Runnable.class), anyLong(), any(TimeUnit.class)); } @Test @@ -453,17 +435,14 @@ void testGlobalDiversionWithNonZeroLoadEnablesSessions() throws Exception { } /** - * Restarts the manager against a server that reports session_load=0, with the supplied session + * Restarts the manager with {@code OVERRIDE_USE_DEFAULT_SYS_PROP_KEY=true} and the supplied session * configuration supplied as a sys-prop override, and returns the resolved config. */ private ClientConfiguration startWithSessionOverride( SessionClientConfiguration.Builder sessionOverride) throws Exception { - ClientConfiguration.Builder serverConfig = manager.getDefaultConfig().toBuilder(); - serverConfig.getSessionConfigurationBuilder().setSessionLoad(0); - service.config.set(serverConfig.build()); - manager.close(); Properties sysProps = new Properties(); + sysProps.setProperty(ClientConfigurationManager.OVERRIDE_USE_DEFAULT_SYS_PROP_KEY, "true"); sysProps.setProperty( ClientConfigurationManager.OVERRIDE_SYS_PROP_KEY, TextFormat.printer() @@ -620,9 +599,10 @@ public void onChange(SessionClientConfiguration newValue) { @Test void disableDirectPathFallbackTest() throws Exception { - Properties sysProps = new Properties(); - sysProps.setProperty( + Properties overrideSysProps = new Properties(); + overrideSysProps.setProperty( ClientConfigurationManager.DISABLE_DIRECT_ACCESS_FALLBACK_SYS_PROP_KEY, "true"); + overrideSysProps.setProperty(ClientConfigurationManager.OVERRIDE_USE_DEFAULT_SYS_PROP_KEY, "true"); String clientConfigOverrides = TextFormat.printer() .printToString( @@ -639,21 +619,39 @@ void disableDirectPathFallbackTest() throws Exception { .setDirectAccessWithFallback( DirectAccessWithFallback.getDefaultInstance()))) .build()); - sysProps.setProperty(ClientConfigurationManager.OVERRIDE_SYS_PROP_KEY, clientConfigOverrides); + overrideSysProps.setProperty( + ClientConfigurationManager.OVERRIDE_SYS_PROP_KEY, clientConfigOverrides); - try (ClientConfigurationManager fallbackDisabledManager = + try (ClientConfigurationManager overrideFallbackDisabledManager = new ClientConfigurationManager( - sysProps, FEATURE_FLAGS, CLIENT_INFO, channelProvider, noopDebugTracer, mockExecutor)) { - - // Check initial default config with override has fallback disabled and direct_access_only set + overrideSysProps, + FEATURE_FLAGS, + CLIENT_INFO, + channelProvider, + noopDebugTracer, + mockExecutor)) { + // Check initial config with override has fallback disabled and direct_access_only set SessionClientConfiguration.ChannelPoolConfiguration initialChannelConfig = - fallbackDisabledManager + overrideFallbackDisabledManager .getClientConfiguration() .getSessionConfiguration() .getChannelConfiguration(); assertThat(initialChannelConfig.hasDirectAccessOnly()).isTrue(); assertThat(initialChannelConfig.hasDirectAccessWithFallback()).isFalse(); + } + // Also verify server-fetched config (without local override) converts direct_access_with_fallback + Properties serverSysProps = new Properties(); + serverSysProps.setProperty( + ClientConfigurationManager.DISABLE_DIRECT_ACCESS_FALLBACK_SYS_PROP_KEY, "true"); + try (ClientConfigurationManager fallbackDisabledManager = + new ClientConfigurationManager( + serverSysProps, + FEATURE_FLAGS, + CLIENT_INFO, + channelProvider, + noopDebugTracer, + mockExecutor)) { // Start manager and fetch server config (which sends direct_access_with_fallback) ClientConfiguration fetchedConfig = fallbackDisabledManager.start().get(); SessionClientConfiguration.ChannelPoolConfiguration fetchedChannelConfig =