Skip to content

fix: addOrSet(serverGroup) writes through one client per server group - #202

Merged
bihaoxwork merged 1 commit into
masterfrom
fix/addorset-one-client-per-server-group
Sep 30, 2026
Merged

bihaoxwork merged 1 commit into
masterfrom
fix/addorset-one-client-per-server-group

Conversation

@bihaoxwork

@bihaoxwork bihaoxwork commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Summary

EVCacheInternal.addOrSet(..., serverGroup(s), destinationIps) writes each key poolSize times to the same server group, not once. This PR changes it to write through a single client per server group, which is what regular writes already do.

Background: how the pool is laid out

EVCacheClientPool creates poolSize EVCacheClient instances for every server group (EVCacheClientPool.refresh). Each client has its own connections to every node in that server group, so the clients differ only in connection and queue; they all route a given key to the same node.

memcachedInstancesByServerGroup
  sg-a → [c0, c1, ..., c(poolSize-1)]
  sg-b → [c0, c1, ..., c(poolSize-1)]

The extra clients are there to spread load across more connections. They are not meant to multiply writes.

Regular writes pick one client per server group

EVCacheImpl write methods (set/add/replace/delete/touch/...) get their clients from _pool.getEVCacheClientForWrite() → getAllWriteClients(). setupAllEVCacheWriteClientsArray() transposes the pool into poolSize arrays, where array i holds the i-th client of each server group:

array0 = [sg-a.c0, sg-b.c0]
array1 = [sg-a.c1, sg-b.c1]
...

Each write takes one array from a CircularIterator. The result is exactly one write per server group, rotating across the pool's connections. Write-only server groups (getWriteOnlyEVCacheClients()) likewise pick one client per server group.

The bug: addOrSet(serverGroup) uses every client

EVCacheInternalImpl.addOrSet (used by cache warmers to write into a specific server group) instead took the whole client list for the target server group:

Map<ServerGroup, List<EVCacheClient>> clientsByServerGroup = _pool.getAllInstancesByZone();
List<EVCacheClient> evCacheClients = clientsByServerGroup.entrySet().stream()
        .filter(entry -> serverGroups.contains(entry.getKey().getName()))
        .map(Map.Entry::getValue)
        .flatMap(List::stream)          // keeps all poolSize clients
        .collect(Collectors.toList());
...
return this.add(key, value, null, timeToLive, policy, evCacheClientsArray, evCacheClientsArray.length, false);

EVCacheClientUtil.add then loops over the array and calls client.add(...) on each client. Every client derives the same key and routes it to the same primary node, so one addOrSet call sends the same ADD (or SET) poolSize times to the same memcached node, and the latch waits for poolSize × serverGroups futures.

The destinationIps filter doesn't help. All clients in a server group share the same node locator, so either all of them match the destination IP or none do.

Impact

With replaceItem=false (ADD), the first write to reach the node stores the item and the other poolSize - 1 return NOT_STORED. The duplicates add no data, but they:

  • multiply write load on the destination server group (client queues, network, server cmd_set) by poolSize;
  • under bulk writes such as replica warming, fill client operation queues and cause sustained timeouts. When all attempts for a key time out, that key is never written.

In a large replica warming with poolSize=6, we saw roughly 6 ADD calls per key. Most returned NOT_STORED, a significant share timed out, and some keys ended up missing on the destination.

The default poolSize is 1, where the old and new behavior are identical. Only deployments that set <app>.EVCacheClientPool.poolSize > 1 are affected.

Change

  • Add EVCacheInternalImpl.selectOneClientPerServerGroup(clientsByServerGroup, serverGroups). It returns one client for each matching, non-empty server group, chosen with ThreadLocalRandom so load still spreads across the pool's connections.
  • addOrSet uses it in place of flatMap(List::stream). Everything else stays the same:
    • the destinationIps filter runs on the selected clients, and gives the same result as before because all clients in a server group share the node locator;
    • replaceItem still selects set vs add (with fixup=false);
    • duet server groups returned by getAllInstancesByZone() are separate entries, so each still gets one write.
  • No public API change; the helper is package-private and static.
Before After
Writes per key poolSize × target server groups 1 × target server groups
Latch count poolSize × target server groups target server groups
poolSize = 1 1 per server group unchanged
Regular EVCacheImpl writes 1 per server group unchanged (not touched)

The other callers of getAllInstancesByZone() are left alone on purpose. metaGetPerClient / metaDebugPerClient are per-client debug reads, and pingServers is a health check.

Tests

  • New EVCacheInternalImplTest, registered in test-suite.xml:
    • one client for the target server group regardless of pool size, and that client comes from the target group;
    • one client per server group when multiple groups are targeted, with non-target groups excluded;
    • an empty result when no server group matches or the matching group has no clients.
  • ./gradlew :evcache-core:test passes on JDK 21. With the old flatMap behavior restored, the two selection tests fail.

EVCacheClientPool creates poolSize EVCacheClients per server group, all
connected to the same nodes. addOrSet(..., serverGroup(s), destinationIps)
flat-mapped every client of the target server groups and wrote through
each one, so a single call sent the same ADD/SET poolSize times to the
same node. With add semantics, the first write stores the item and the
rest come back NOT_STORED; the extra writes only add load and can time
out.

Regular writes don't have this problem: getEVCacheClientForWrite()
picks one client per server group. Do the same in addOrSet by selecting
one client per matching server group. The destinationIps filter is
unchanged; all clients in a server group share the same node locator,
so the choice of client doesn't affect it.
@bihaoxwork
bihaoxwork merged commit 64c7a35 into master Sep 30, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants