This repository was archived by the owner on Jul 22, 2026. It is now read-only.
Add potential clients cache to avoid recomputation on every request - #1151
Merged
Conversation
TylerHorth
marked this pull request as ready for review
February 27, 2026 02:21
TylerHorth
temporarily deployed
to
jfrog-publish
February 27, 2026 23:46 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
golthitarun
reviewed
Mar 3, 2026
| * On rebuild, the entire inner map for a service is replaced atomically, which | ||
| * naturally removes stale entries for partitions that no longer exist. | ||
| */ | ||
| private final ConcurrentMap<String, Map<String, Map<Integer, Map<URI, TrackerClient>>>> _potentialClientsCache; |
Contributor
There was a problem hiding this comment.
Looks like outermap is only concurrentMap, would this cause any race conditions on inner maps ?
Contributor
Author
There was a problem hiding this comment.
The inner maps are immutable, so no risk for race conditions
TylerHorth
force-pushed
the
thorth/potential-clients-cache
branch
from
April 8, 2026 18:00
ec6dc7c to
1bddfed
Compare
TylerHorth
force-pushed
the
thorth/potential-clients-cache
branch
from
April 14, 2026 16:58
6de10e7 to
8e0df88
Compare
TylerHorth
temporarily deployed
to
jfrog-publish
April 14, 2026 17:01 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
…okup SimpleLoadBalancer.getPotentialClients() previously ran O(n) loops on every request to build the Map<URI, TrackerClient> by iterating all URIs, filtering banned hosts, and applying subsetting. This work is now done eagerly in response to state-change events (URI/service/cluster property updates) and cached in SimpleLoadBalancerState, making the request path a single ConcurrentHashMap lookup. The feature is gated behind a new config flag `enablePotentialClientsCache` (default: false) plumbed through D2ClientConfig -> D2ClientBuilder -> ZKFSTogglingLoadBalancerFactoryImpl -> SimpleLoadBalancerState. When disabled, the existing O(n) fallback logic is used unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
30 tests verifying the precomputed cache produces results identical to the original O(n) per-request computation. Uses DataProvider to sweep across scenarios varying URI count (1-20), cluster-banned count (0-10), and service-banned count (0-5). Each scenario creates two load balancers (cache enabled vs disabled) and asserts equivalence. Also includes lifecycle tests for cache rebuild on URI/ban-list changes, invalidation on service removal, and multi-service-per-cluster behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The LogUtil.debug() check calls Logger.isDebugEnabled() which is expensive on the hot request path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous assertPotentialClientsEquivalent reimplemented the filtering logic in the test rather than exercising the actual old code path. Replace with drainAllRoutableHosts which calls getClient() 1000 times with varied request URIs to discover all hosts each LB can route to, then compares the full sets. The cached LB hits the precomputed cache; the uncached LB falls through to the original O(n) fallback in SimpleLoadBalancer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ries Replace flat ConcurrentMap<PotentialClientsKey, ...> with nested serviceName -> scheme -> partitionId -> Map<URI, TrackerClient>. On rebuild, the entire inner map for a service is replaced atomically, which naturally drops entries for partitions that no longer exist. Invalidation becomes a single O(1) remove(serviceName) instead of scanning all keys with removeIf. PotentialClientsKey class is deleted. Adds test that verifies stale partition entries are removed when a URI update drops a partition. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
47 tests covering all cache scenarios: end-to-end equivalence with exhaustive host draining (24 scenarios), equivalence after state changes, URI/service/cluster property lifecycle, stale partition removal, multi-service and multi-cluster isolation, overlapping bans, cache immutability, disabled fallback, rapid updates, and boundary lookups. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…y paths The enablePotentialClientsCache flag was only plumbed through the ZK factory path (ZKFSTogglingLoadBalancerFactoryImpl), so the potential clients cache was never activated when using XDS or LastSeen load balancers. This propagates the flag through all factory code paths: - XdsFsTogglingLoadBalancerFactory: add enablePotentialClientsCache parameter and pass it to SimpleLoadBalancerState - XdsLoadBalancerWithFacilitiesFactory: pass config.enablePotentialClientsCache when constructing XdsFsTogglingLoadBalancerFactory - LastSeenBalancerWithFacilitiesFactory: pass config.enablePotentialClientsCache when constructing SimpleLoadBalancerState - ZKFSTogglingLoadBalancerFactoryImpl: add new backwards-compatible constructor with enablePotentialClientsCache appended (preserving existing constructor) Adds parameterized tests covering ZK and XDS factory paths with cache enabled/disabled, exercising the real factory classes and verifying behavior through getPotentialClients(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TylerHorth
force-pushed
the
thorth/potential-clients-cache
branch
from
April 22, 2026 00:52
8e0df88 to
a3ea0d9
Compare
| return result; | ||
| } | ||
|
|
||
| private Map<URI, TrackerClient> buildPotentialClientsSubsetting( |
Contributor
There was a problem hiding this comment.
Is it possible to leverage getPotentialClientsSubsetting for re-use here in some capacity?
Contributor
Author
There was a problem hiding this comment.
The implementation isn't identical, but yes we could likely refactor to reuse some of previous code. However, then we'd need to do dark cluster testing again. I'd rather:
- Use current solution
- Ramp globally via global config
- Once stable, delete the old code
What do you think?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SimpleLoadBalancer.getPotentialClients()is a hotspot in production profiles. It runs O(n) loops on every request (n = total hosts in partition) to build aMap<URI, TrackerClient>by iterating all URIs, filtering banned hosts, resolving TrackerClients, and applying subsetting. All inputs to this computation change only on state events (host join/leave, config updates), yet the work was repeated on every request.This PR moves the O(n) computation into the state-change event handlers in
SimpleLoadBalancerState, precomputing and caching the result in aConcurrentHashMap. The request path becomes a single O(1) map lookup. The feature is gated behind a new config flagenablePotentialClientsCache(default:false) so it can be rolled out incrementally.Key changes:
SimpleLoadBalancerStatemaintains a_potentialClientsCachekeyed by(serviceName, scheme, partitionId), rebuilt eagerly by subscriber callbacks after URI, service, or cluster property updatesSimpleLoadBalancer.getPotentialClients()checks the cache first, falling back to the existing O(n) logic when the cache is disabled or returns nullenablePotentialClientsCacheflag plumbed throughD2ClientConfig→D2ClientBuilder→ZKFSTogglingLoadBalancerFactoryImpl→SimpleLoadBalancerStateLogUtil.debug()calls from the hot path (Logger.isDebugEnabled()is costly at high call rates)Testing Done
PotentialClientsCacheTestverify equivalence between the cached and uncached code paths across 12 input scenarios varying URI count (1–20), cluster-banned count (0–10), and service-banned count (0–5)