[v2/resolve] Routing Refactoring & Header Overrides#1925
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the routing logic for V2Resolve requests by introducing a shouldRouteResolveToDispatcher helper, which handles routing for place, topic, and indicator resolvers, and supports request-time backend overrides via the X-V2Resolve-Indicator-Backend header. It also cleans up the SpannerDataSource initialization by removing the unused enableSpannerSearchEmbeddings parameter and adds architectural documentation on flag usage. The review feedback highlights a potential initialization bug in cmd/main.go where s.useSpannerGraph might be incorrectly set, and suggests validating the new header values to prevent silent fallbacks on typos, along with adding corresponding test coverage.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| switch backendHeader { | ||
| case util.V2ResolveIndicatorBackendSpanner: | ||
| // Force Spanner: Fail fast if Spanner backend is not configured | ||
| if !s.useSpannerGraph { |
There was a problem hiding this comment.
Potential Bug: In cmd/main.go (line 473), NewMixerServer is initialized using *useSpannerGraph (the CLI flag) instead of shouldUseSpannerGraph (which also accounts for flags.UseSpannerGraph from feature flags).
As a result, if Spanner is enabled via feature flags but the CLI flag is false, s.useSpannerGraph will be false here, causing this check to incorrectly fail-fast even though the Spanner backend is fully initialized and running.
Please update cmd/main.go to pass shouldUseSpannerGraph to NewMixerServer.
| switch backendHeader { | ||
| case util.V2ResolveIndicatorBackendSpanner: | ||
| // Force Spanner: Fail fast if Spanner backend is not configured | ||
| if !s.useSpannerGraph { | ||
| slog.Error("Spanner backend requested via header, but Spanner is not enabled on this server") | ||
| return false, status.Errorf(codes.FailedPrecondition, "Spanner backend is not enabled in this mixer") | ||
| } | ||
| return true, nil | ||
| case util.V2ResolveIndicatorBackendLegacy: | ||
| // Force Legacy Remote Service | ||
| return false, nil | ||
| default: | ||
| // Default: use Spanner if configured AND default routing flag is true | ||
| return s.useSpannerGraph && s.flags.EnableSpannerSearchEmbeddings, nil | ||
| } |
There was a problem hiding this comment.
If the client provides an invalid or misspelled value for the X-V2Resolve-Indicator-Backend header (e.g., "spaner"), the switch statement will silently fall through to the default case and use the default routing.
To prevent silent failures and help clients debug typos, consider validating the header value and returning an InvalidArgument error if it is non-empty and unrecognized.
switch backendHeader {
case util.V2ResolveIndicatorBackendSpanner:
// Force Spanner: Fail fast if Spanner backend is not configured
if !s.useSpannerGraph {
slog.Error("Spanner backend requested via header, but Spanner is not enabled on this server")
return false, status.Errorf(codes.FailedPrecondition, "Spanner backend is not enabled in this mixer")
}
return true, nil
case util.V2ResolveIndicatorBackendLegacy:
// Force Legacy Remote Service
return false, nil
case "":
// Default: use Spanner if configured AND default routing flag is true
return s.useSpannerGraph && s.flags.EnableSpannerSearchEmbeddings, nil
default:
return false, status.Errorf(codes.InvalidArgument, "invalid backend requested via header: %s", backendHeader)
}| }, | ||
| } |
There was a problem hiding this comment.
To verify the validation of the X-V2Resolve-Indicator-Backend header, consider adding a test case for an invalid header value.
},
{
desc: "Indicator resolver - Invalid header value -> error",
useSpannerGraph: true,
resolver: resolve.ResolveResolverIndicator,
headerVal: "invalid-backend",
wantErr: true,
},
}1 participant
Overview
This PR refactors
V2Resolverouting logic to support request-time backend overrides for indicator resolution, simplifies the configuration model by managing default routing via a feature flag, and forces topic resolution to bypass the dispatcher (routing to local in-memory cache).Key Changes
internal/server/handler_v2.go): IntroducedshouldRouteResolveToDispatcherhelper.shouldDivertV2).false) to utilize localTopicCache, preventing empty candidates when Spanner is enabled.X-V2Resolve-Indicator-Backendheader (spanner/legacy). Defaults to Spanner if Spanner is configured andEnableSpannerSearchEmbeddingsflag is true.internal/server/spanner/datasource.go): Removed theenableSpannerSearchEmbeddingsflag fromSpannerDataSourceconstructor and methods. The datasource execution capability is now determined purely by Spanner configuration at startup, with routing managed in the handler.internal/featureflags/featureflags.go): UpdatedEnableSpannerSearchEmbeddingsflag description to define its role as the default routing toggle for indicators.internal/server/handler_v2_test.go): AddedTestShouldRouteResolveToDispatcherto verify routing combinations and fail-fast behavior.cmd/main.go,test/setup.go, spanner unit/golden tests): RefactoredNewSpannerDataSourcecall sites to remove the flag argument.Integration tests
Verified routing logic E2E with local NL embeddings server and test Spanner instances:
spanner/legacy/ absent) and feature flag defaults behave as expected. Fail-fast error verified.dc/topic/Agriculturesuccessfully bypasses dispatcher and returns identical resolved candidates regardless of whether cache is loaded from Spanner or Bigtable.Follow-up Work (PR 2)
We will migrate
resolver=topicrequests to resolve through the dispatcher when Spanner is enabled. This will involve:SpannerDataSourceto delegate topic resolution toTopicExpandervia closure-based lazy injection to break circular startup dependencies.TopicCacheManagerinitialization from the embeddings resolver flag.topicresolver requests to the dispatcher when Spanner is enabled.