Repository navigation
fix(bigtable): copy Metadata per GetClientConfiguration call - #14950
Conversation
There was a problem hiding this comment.
Code Review
This pull request improves the ClientConfigurationManager by preventing metadata accumulation across polls, avoiding retry amplification during background polls when falling back to the default configuration, and introducing a mockable Clock for better testability. It also adds comprehensive unit tests to verify these behaviors. The reviewer's feedback highlights an issue in the logging implementation where concatenating a Throwable into a log message discards the stack trace and suggests a Java 8-compatible logging pattern to preserve the stack trace and avoid unnecessary string concatenation.
| if (willRetry) { | ||
| logger.log( | ||
| Level.FINE, | ||
| () -> | ||
| "Failed to fetch client config (attempt " | ||
| + (attempt + 1) | ||
| + "), retrying: " | ||
| + throwable); | ||
| } else if (!isInitialPoll && !shouldReset) { |
There was a problem hiding this comment.
Concatenating the throwable into the log message string only prints its toString() representation, which discards the stack trace. For Level.FINE logs, having the full stack trace of the failed RPC is extremely valuable for debugging network or gRPC issues. Additionally, the lambda-based logging overload log(Level, Throwable, Supplier) is only available in Java 9+. If this library targets Java 8 compatibility, using standard log(Level, String, Throwable) wrapped in logger.isLoggable(Level.FINE) is safer and avoids string concatenation overhead when FINE logging is disabled.
| if (willRetry) { | |
| logger.log( | |
| Level.FINE, | |
| () -> | |
| "Failed to fetch client config (attempt " | |
| + (attempt + 1) | |
| + "), retrying: " | |
| + throwable); | |
| } else if (!isInitialPoll && !shouldReset) { | |
| if (willRetry) { | |
| if (logger.isLoggable(Level.FINE)) { | |
| logger.log( | |
| Level.FINE, | |
| "Failed to fetch client config (attempt " + (attempt + 1) + "), retrying", | |
| throwable); | |
| } | |
| } else if (!isInitialPoll && !shouldReset) { |
ClientConfigurationManager previously passed the same mutable Metadata instance to ClientCall.start() on every GetClientConfiguration poll and retry. Because ClientCall.start() takes ownership of the Metadata passed to it and channel ClientInterceptors may mutate it in place, per-RPC headers attached by interceptors accumulated across polls until outbound header encoding exceeded HTTP/2 SETTINGS_MAX_HEADER_LIST_SIZE. Copy the base Metadata into a fresh Metadata instance per ClientCall in sendRequest(), matching GrpcClientCalls, BigtableChannelPrimer, and SessionPoolImpl.
8dfb57d to
2456fe2
Compare
🤖 I have created a release *beep* *boop* --- <details><summary>1.97.0</summary> ## [1.97.0](v1.96.0...v1.97.0) (2026-11-12) ### ⚠ BREAKING CHANGES * **shopping-merchant:** remove discontinued Merchant API v1beta ([#14942](#14942)) ### Features * **bigquery-jdbc:** adds BigQuery graph integration to DatabaseMetadata methods ([#14930](#14930)) ([950b81b](950b81b)) * **gax:** implement resumable upload session resumption ([#14677](#14677)) ([24728d6](24728d6)) * **grpc-gcp:** add built-in channel_count metric for client library pools ([#14947](#14947)) ([712f043](712f043)) * **grpc-gcp:** add built-in channel_load metric for client library pools ([#14985](#14985)) ([9f87888](9f87888)) * **shopping-merchant:** remove discontinued Merchant API v1beta ([#14942](#14942)) ([2947dfd](2947dfd)) * **storage:** add StorageErrorTypeMapper for standard metrics ([#14694](#14694)) ([a9ac393](a9ac393)) * **storage:** add StorageMetricsContext for operation metrics state ([#14728](#14728)) ([edea395](edea395)) * **storage:** wire logical operation metrics in Retrying and StorageOptions ([#14695](#14695)) ([589c66c](589c66c)) * update API sources and regenerate ([#14958](#14958)) ([75944f3](75944f3)) ### Bug Fixes * **auth:** verify public/private key match with retries in mTLS keystore loading ([#14829](#14829)) ([096dbd6](096dbd6)) * **bigquery:** set QueryResultsFormat.ARROW in QueryArrow sample ([#14960](#14960)) ([c98e7c3](c98e7c3)) * **bigtable:** avoid triggering per-RPC credential session fallback on default call context ([#14961](#14961)) ([eb06d2c](eb06d2c)) * **bigtable:** copy Metadata per GetClientConfiguration call ([#14950](#14950)) ([278fe2f](278fe2f)) * **bigtable:** disable SwitchingChannelPool when ChannelPoolConfiguration is empty ([#14911](#14911)) ([fa24702](fa24702)) * **bigtable:** handle empty SessionPoolConfiguration in SessionPoolImpl and PoolSizer ([#14912](#14912)) ([cc7b235](cc7b235)) * **bigtable:** replace SessionPoolImpl Watchdog with per-session closeTimeout ([#14977](#14977)) ([a15acfb](a15acfb)) * **spanner-jdbc:** report database default isolation level ([#14929](#14929)) ([7e7595a](7e7595a)) * **spanner:** fix PG SHOW isolation level statements ([#14904](#14904)) ([bc917ed](bc917ed)) ### Dependencies * bump grpc to 1.85.1, opentelemetry to 1.67.0, and guava to 33.7.2-jre ([#14957](#14957)) ([1a27a00](1a27a00)) * bump guava to 33.7.1-jre and jspecify to 1.0.1 ([#14937](#14937)) ([1441527](1441527)) ### Documentation * **spanner:** update CHANGELOG.md for java spanner releases ([#14935](#14935)) ([956681e](956681e)) * **storage:** add Rapid Cache samples ([#14886](#14886)) ([e714f62](e714f62)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Problem
ClientConfigurationManagerconstructs a singleio.grpc.Metadatainstance in its constructor and passes that same mutable reference toClientCall.start(listener, metadata)on everyGetClientConfigurationpoll and retry. Ingrpc-java,ClientCall.start()takes ownership of theMetadatainstance, and channelClientInterceptors wraprealChannelaboveRetriableStreamand mutateheadersin place before any defensive copy is made. Whenever a channelClientInterceptor(for exampleMetadataUtils.newAttachHeadersInterceptoror a tracing interceptor callingheaders.put(...)instart()) appends per-RPC headers withoutdiscardAll, duplicate header entries accumulate onClientConfigurationManager.metadataevery 5 minutes until outbound header encoding exceeds the server's HTTP/2SETTINGS_MAX_HEADER_LIST_SIZE(64 KiB), causing Netty'sHpackEncoderto fail every subsequent poll locally withStatus.INTERNAL: http2 exception(HeaderListSizeException) — survivingreplaceChannel()until JVM restart.Fix
this.metadatainto a freshMetadatainstance perClientCallinClientConfigurationManager.sendRequest(), matchinggax-grpc'sGrpcClientCalls,BigtableChannelPrimer, andSessionPoolImpl.Testing
metadataDoesNotAccumulateAcrossPollsWithHeaderAttachingInterceptorverifying 26 consecutive polls against a server advertisingmaxInboundMetadataSize(1024)with a header-appendingClientInterceptorsucceed without header accumulation (1copy of each header per poll on the wire).