[fast-client] Make dictionary fetch replica-aware and stop dropping 404s

[ymuppala]

Problem Statement

The Fast Client fetches the zstd compression dictionary through the cluster-wide server D2 service:

String url = QueryAction.DICTIONARY.toString().toLowerCase() + "/" + storeName + "/" + version;
d2TransportClient.get(url)...

D2 load balances across every server in the cluster, so the request can land on a server that does not host the store-version being fetched. The server serves this endpoint purely from node-local StoreVersionState (StorageReadRequestHandler#handleDictionaryFetchRequest -> StorageMetadataService#getStoreVersionCompressionDictionary); a missing entry becomes a BinaryResponse with NOT_FOUND. There is no Kafka fallback and no forwarding. When the D2 pool is much larger than the replica set of the version, most of these requests 404.

The 404 was then dropped. TransportClientCallback#completeFuture maps 404 to valueFuture.complete(null), and the dictionary callback dereferenced that null:

byte[] dictionary = response.getBody();   // NPE inside whenComplete

The NPE landed on the discarded derived stage, so compressionDictionaryFuture was never completed. refresh() then blocked on dictionaryFetchFuture.get(ZSTD_DICT_FETCH_TIMEOUT_IN_SECONDS) and failed 10s later with "Dictionary fetch operation could not complete in time", which makes a routing bug look like a slow server. On the initial refresh this fails start(); on a periodic refresh the new version is never adopted. Raising the timeout does not help either defect.

The router is not affected: DictionaryRetrievalService#getOnlineInstance picks a ready-to-serve replica of that specific version, and treats a non-200 as an explicit failure.

Solution

Two independent fixes in RequestBasedMetadata:

1. Replica-aware selection. The dictionary request now targets a replica taken from the routing table of the fetched version, mirroring what the router does, and retries against a different replica on failure (capped at min(replicas.size(), 3)). Replicas are shuffled so a fleet of clients does not converge on one host. This uses the already-present r2TransportClient, the same one used for H2 connection warmup, so no new plumbing. The fetch moved to after the routing info in the metadata response is parsed, since it now depends on it.

2. Reliable completion. The dictionary future is always completed. A 404/null response, a null or empty body, or any throwable raised inside the callback completes it exceptionally with the real cause instead of being swallowed.

The 404 -> null mapping in TransportClientCallback is deliberately left alone: on the single-get path that is the correct key-not-found signal, so changing it there would have a much wider blast radius.

Code changes

• Added new code behind a config. If so list the config names and their default values in the PR description.
• Introduced new log lines.
  • Confirmed if logs need to be rate limited to avoid excessive logging. The warn lines are bounded by the retry cap (at most 3 per dictionary fetch) and a dictionary fetch only happens on a refresh that observes a new ZSTD version.

Concurrency-Specific Checks

Both reviewer and PR author to verify

• Code has no race conditions or thread safety issues.
• Proper synchronization mechanisms (e.g., synchronized, RWLock) are used where needed. The retry chain runs on the transport callback thread and carries no shared mutable state beyond the existing VeniceConcurrentHashMap.
• No blocking calls inside critical sections that could lead to deadlocks or performance degradation. Retries are chained asynchronously in whenComplete; the only wait is the pre-existing bounded get(ZSTD_DICT_FETCH_TIMEOUT_IN_SECONDS).
• Verified thread-safe collections are used (e.g., ConcurrentHashMap, CopyOnWriteArrayList).
• Validated proper exception handling in multi-threaded code to avoid silent thread termination. This is the core of the fix: the callback body is wrapped so nothing thrown inside it can orphan the future.

How was this PR tested?

• New unit tests added.
• New integration tests added.
• Modified or extended existing tests.
• Verified backward compatibility (if applicable).

Four new tests in RequestBasedMetadataTest, each verified to fail against the previous implementation (three of them by hanging until the test timeout, which is the orphaned-future symptom):

Test	Covers
testDictionaryFetchTargetsReplicaHostingTheStoreVersion	Captures every RestRequest and asserts dictionary requests only target replicas of the version, and that the D2 client is never used for the dictionary path
testDictionaryFetchRetriesAnotherReplicaOn404	Only one replica serves the dictionary, the other 404s; the fetch still succeeds
testDictionaryFetch404IsNotMaskedAsATimeout	All replicas 404; fails fast with the 404 cause preserved, and with no TimeoutException or NullPointerException
testDictionaryFetchRejectsEmptyBody	A 200 with an empty body fails instead of caching an unusable dictionary

RequestBasedMetadataTestUtils was updated so the mock R2 client serves the DICTIONARY endpoint, and the now-dead D2 dictionary stubs were removed from the existing tests.

Full com.linkedin.venice.fastclient.* suite: 354 tests, 1 failure in testRequestBasedMetadataOnDemandRefresh, which reproduces identically on unmodified main and passes in isolation (pre-existing timing flake, untouched by this PR).

Does this PR introduce any user-facing or breaking changes?

• No. You can skip the rest of this section.
• Yes. Clearly explain the behavior change and its impact.

No API or config change. The observable difference is in failure reporting: a dictionary fetch that previously failed with a generic "could not complete in time" after 10s now fails fast with the underlying cause (for example the 404 and the replica URL that returned it). Cases that previously failed only because the request was routed to a server not hosting the store-version now succeed.

[Copilot]

🟡 Changes recommended

Unresolved findings remain around synchronous request failures, gRPC R2-client initialization, failure logging, and replica URL matching in tests.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates Fast Client ZSTD dictionary retrieval to target serving replicas and preserve fetch failures.

Changes:

• Added replica-aware selection with bounded retries.
• Added validation for 404, empty responses, and callback failures.
• Updated R2 mocks and dictionary-fetch tests.

File summaries

File	Summary
clients/venice-client/src/test/java/com/linkedin/venice/fastclient/meta/RequestBasedMetadataTestUtils.java	Adds R2 dictionary endpoint mocks and replica-aware fixtures.
clients/venice-client/src/test/java/com/linkedin/venice/fastclient/meta/RequestBasedMetadataTest.java	Adds coverage for routing, retries, and failure handling.
clients/venice-client/src/main/java/com/linkedin/venice/fastclient/meta/RequestBasedMetadata.java	Implements replica selection and dictionary retry handling.

Review details

Suppressed comments (3)

clients/venice-client/src/main/java/com/linkedin/venice/fastclient/meta/RequestBasedMetadata.java:862

• This path now assumes the shared R2 client is always available, but gRPC-enabled ClientConfig explicitly permits its top-level r2Client to be null; the non-storage R2 client is held in GrpcClientConfig instead. In that supported configuration r2TransportClient was constructed with null, so a ZSTD dictionary refresh dereferences null and never succeeds. Initialize this transport from the active gRPC pass-through client when useGrpc is true (or otherwise make the shared transport use that client).

    r2TransportClient.get(url).whenComplete((response, throwable) -> {

clients/venice-client/src/main/java/com/linkedin/venice/fastclient/meta/RequestBasedMetadata.java:911

• Because this new path completes the dictionary future exceptionally for 404s and empty bodies, dictionaryFetchFuture.get() reaches the existing ExecutionException branch in updateCache, which logs every such failure as “could not complete in time.” The routing failure is therefore still reported as a timeout in logs, contrary to this PR’s failure-reporting goal; distinguish TimeoutException from ExecutionException and log the latter’s cause instead.

      compressionDictionaryFuture.completeExceptionally(new VeniceClientException(message, failure));

clients/venice-client/src/test/java/com/linkedin/venice/fastclient/meta/RequestBasedMetadataTestUtils.java:200

• These new matchers inspect URI.getPath() for the replica name, but production metadata supplies fully qualified http(s)://host:port replica URLs, whose host is not part of the path. With realistic metadata the serving-replica matcher would never match and every request would be treated as a 404, so this test only works because the fixture uses bare host1/host2 strings. Match the URI host/authority or the full replica URL so the retry test models production routing.

            argThat(argument -> isDictionaryRequest(argument) && argument.getURI().getPath().contains(servingReplica)),

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.