From 1ba404689ff1aff6dc47923ec7e8951d1c7eb577 Mon Sep 17 00:00:00 2001 From: Bihao Xu Date: Wed, 27 May 2026 16:52:34 -0700 Subject: [PATCH 1/4] fix: sync getBulk correctly decodes mixed hashed and plain keys Refactor getBulkData to split hashed vs plain keys and route each set through the appropriate transcoder path, fixing incorrect decoding when a single bulk request contains both hashed and unhashed keys. Re-enable the previously disabled getBulk test path in EVCacheTestDI. Co-authored-by: Bihao Xu --- .../netflix/evcache/test/EVCacheTestDI.java | 7 +- .../java/com/netflix/evcache/EVCacheImpl.java | 82 +++++++------------ .../netflix/evcache/pool/EVCacheClient.java | 29 +++++++ 3 files changed, 62 insertions(+), 56 deletions(-) diff --git a/evcache-client/test/com/netflix/evcache/test/EVCacheTestDI.java b/evcache-client/test/com/netflix/evcache/test/EVCacheTestDI.java index e69280e3..ba3cfcb7 100644 --- a/evcache-client/test/com/netflix/evcache/test/EVCacheTestDI.java +++ b/evcache-client/test/com/netflix/evcache/test/EVCacheTestDI.java @@ -449,13 +449,12 @@ private void testWithMixedKeysAndCustomTranscoder() throws Exception { // async bulk get for (int op : new int[]{0, 1}) { - Map results = new HashMap<>(); + Map results; if (op == 0) { CompletableFuture> future = evCache.getAsyncBulk(kv.keySet().toArray(new String[0])); results = future.get(10000, TimeUnit.MILLISECONDS); - // } else { - // TODO: getBulk api is known to be broken for un-hashed keys not decoding correctly when request contains both hashed and unhashed keys - // results = evCache.getBulk(kv.keySet().toArray(new String[0])); + } else { + results = evCache.getBulk(kv.keySet().toArray(new String[0])); } for (Map.Entry result : results.entrySet()) { diff --git a/evcache-core/src/main/java/com/netflix/evcache/EVCacheImpl.java b/evcache-core/src/main/java/com/netflix/evcache/EVCacheImpl.java index 67f7a1fd..19f7e42b 100644 --- a/evcache-core/src/main/java/com/netflix/evcache/EVCacheImpl.java +++ b/evcache-core/src/main/java/com/netflix/evcache/EVCacheImpl.java @@ -1969,60 +1969,38 @@ private Map buildKeyValueResult(Map objMap, private Map getBulkData(EVCacheClient client, Collection evcacheKeys, Transcoder tc, boolean throwException, boolean hasZF) throws Exception { try { - boolean hasHashedKey = false; - final Map keyMap = new HashMap<>(evcacheKeys.size() * 2); - for(EVCacheKey evcKey : evcacheKeys) { - String key = evcKey.getCanonicalKey(client.isDuetClient()); - String hashKey = evcKey.getHashKey(client.isDuetClient(), client.getHashingAlgorithm(), client.shouldEncodeHashKey(), client.getMaxDigestBytes(), client.getMaxHashLength(), client.getBaseEncoder()); - if(hashKey != null) { - if (log.isDebugEnabled() && shouldLog()) log.debug("APP " + _appName + ", key [" + key + "], has been hashed [" + hashKey + "]"); - key = hashKey; - hasHashedKey = true; - } - keyMap.put(key, evcKey); - } - if(hasHashedKey) { - final Map objMap = client.getBulk(keyMap.keySet(), evcacheValueTranscoder, throwException, hasZF); - final Map retMap = new HashMap<>((int) (objMap.size() / 0.75) + 1); - for (Map.Entry i : objMap.entrySet()) { - final Object obj = i.getValue(); - if(obj instanceof EVCacheValue) { - if (log.isDebugEnabled() && shouldLog()) log.debug("APP " + _appName + ", The value for key [" + i.getKey() + "] is EVCache Value"); - final EVCacheValue val = (EVCacheValue)obj; - final CachedData cd = new CachedData(val.getFlags(), val.getValue(), CachedData.MAX_SIZE); - final T tVal; - if(tc == null) { - tVal = (T)client.getTranscoder().decode(cd); - } else { - tVal = tc.decode(cd); - } - final EVCacheKey evcKey = keyMap.get(i.getKey()); - if(evcKey.getCanonicalKey(client.isDuetClient()).equals(val.getKey())) { - if (log.isDebugEnabled() && shouldLog()) log.debug("APP " + _appName + ", key [" + i.getKey() + "] EVCacheKey " + evcKey); - retMap.put(evcKey, tVal); - } else { - if (log.isDebugEnabled() && shouldLog()) log.debug("CACHE COLLISION : APP " + _appName + ", key [" + i.getKey() + "] EVCacheKey " + evcKey); - incrementFailure(EVCacheMetricsFactory.KEY_HASH_COLLISION, Call.BULK.name(), EVCacheMetricsFactory.READ); - } - } else { - final EVCacheKey evcKey = keyMap.get(i.getKey()); - if (log.isDebugEnabled() && shouldLog()) log.debug("APP " + _appName + ", key [" + i.getKey() + "] EVCacheKey " + evcKey); - retMap.put(evcKey, (T)obj); - } + final KeyMapDto keyMapDto = buildKeyMap(client, evcacheKeys); + final Set plainKeys = keyMapDto.getPlainKeysMap().keySet(); + final Set hashedKeys = keyMapDto.getHashedKeysMap().keySet(); + + // Preserve chunking behavior for plain-only requests: route through the existing single-transcoder + // path, which is the only one that honors enableChunking. Chunked apps do not use hashed keys. + if (hashedKeys.isEmpty()) { + if (tc == null && _transcoder != null) tc = (Transcoder) _transcoder; + final Map objMap = client.getBulk(plainKeys, tc, throwException, hasZF); + return buildKeyValueResult(objMap, keyMapDto); + } + + final BiPredicate collisionChecker = (hashedKey, decodedKey) -> { + final EVCacheKey evcKey = keyMapDto.getHashedKeysMap().get(hashedKey); + if (evcKey.getCanonicalKey(client.isDuetClient()).equals(decodedKey)) { + if (log.isDebugEnabled() && shouldLog()) + log.debug("APP " + _appName + ", key [" + hashedKey + "] EVCacheKey " + evcKey); + } else { + if (log.isDebugEnabled() && shouldLog()) + log.debug("CACHE COLLISION : APP " + _appName + ", key [" + hashedKey + "] EVCacheKey " + evcKey + " with decodedKey [" + decodedKey + "]"); + incrementFailure(EVCacheMetricsFactory.KEY_HASH_COLLISION, Call.BULK.name(), EVCacheMetricsFactory.READ); + return true; } - return retMap; + return false; + }; - } else { - if(tc == null && _transcoder != null) tc = (Transcoder)_transcoder; - final Map objMap = client.getBulk(keyMap.keySet(), tc, throwException, hasZF); - final Map retMap = new HashMap((int)(objMap.size()/0.75) + 1); - for (Map.Entry i : objMap.entrySet()) { - final EVCacheKey evcKey = keyMap.get(i.getKey()); - if (log.isDebugEnabled() && shouldLog()) log.debug("APP " + _appName + ", key [" + i.getKey() + "] EVCacheKey " + evcKey); - retMap.put(evcKey, i.getValue()); - } - return retMap; - } + final Transcoder valueTranscoder = (tc == null) ? ((_transcoder == null) ? (Transcoder) client.getTranscoder() : (Transcoder) _transcoder) : tc; + if (log.isDebugEnabled() && shouldLog()) + log.debug("fetching bulk data with set of keys containing hashed key(s) {} ", evcacheKeys); + + final Map objMap = client.getBulk(plainKeys, hashedKeys, valueTranscoder, evcacheValueTranscoder, _appName, shouldLog(), collisionChecker, throwException, hasZF); + return buildKeyValueResult(objMap, keyMapDto); } catch (Exception ex) { if (log.isDebugEnabled() && shouldLog()) log.debug("Exception while getBulk data for APP " + _appName + ", key : " + evcacheKeys, ex); if (!throwException || hasZF) return null; diff --git a/evcache-core/src/main/java/com/netflix/evcache/pool/EVCacheClient.java b/evcache-core/src/main/java/com/netflix/evcache/pool/EVCacheClient.java index 3a0dbcd9..cc5a1af7 100644 --- a/evcache-core/src/main/java/com/netflix/evcache/pool/EVCacheClient.java +++ b/evcache-core/src/main/java/com/netflix/evcache/pool/EVCacheClient.java @@ -998,6 +998,35 @@ public Map getBulk(Collection canonicalKeys, Transcoder Map getBulk(Collection plainKeys, Set hashedKeys, + Transcoder valueTranscoder, EVCacheTranscoder evcacheValueTranscoder, + String appName, boolean shouldLog, BiPredicate collisionChecker, + boolean _throwException, boolean hasZF) throws Exception { + try { + if (valueTranscoder == null) valueTranscoder = (Transcoder) getTranscoder(); + final BiPredicate validator = (node, key) -> { + NodeValidationResult result = validateNodeForRead(node, Call.BULK, 2 * maxReadQueueSize.get()); + if (result != NodeValidationResult.OK) { + return false; + } + return true; + }; + return evcacheMemcachedClient + .asyncGetBulk(plainKeys, hashedKeys, valueTranscoder, evcacheValueTranscoder, validator, appName, shouldLog, collisionChecker) + .getSome(bulkReadTimeout.get(), TimeUnit.MILLISECONDS, _throwException, hasZF); + } catch (Exception e) { + if (_throwException) throw e; + return Collections. emptyMap(); + } + } + /** * @Deprecated This method does NOT support a mix of plain and hashed keys in {@code keys}. All keys are * decoded exactly using the given transcoder (note that hashed keys require two step decoding). From fe84979c4bcfc808d763bd77ba1fbfbb8e30abc6 Mon Sep 17 00:00:00 2001 From: Bihao Xu Date: Thu, 4 Jun 2026 11:07:24 -0700 Subject: [PATCH 2/4] Support chunking for EVCache sync-getBulk with mixed key --- .../netflix/evcache/test/EVCacheTestDI.java | 132 +++++++++++++++++- .../java/com/netflix/evcache/EVCacheImpl.java | 8 -- .../netflix/evcache/pool/EVCacheClient.java | 61 ++++++-- 3 files changed, 180 insertions(+), 21 deletions(-) diff --git a/evcache-client/test/com/netflix/evcache/test/EVCacheTestDI.java b/evcache-client/test/com/netflix/evcache/test/EVCacheTestDI.java index ba3cfcb7..e1e4fbb8 100644 --- a/evcache-client/test/com/netflix/evcache/test/EVCacheTestDI.java +++ b/evcache-client/test/com/netflix/evcache/test/EVCacheTestDI.java @@ -261,6 +261,16 @@ public void testAppendOrAdd() throws Exception { } private void refreshEVCache() { + // Close the previous DI container before building a new one. setupEnv() creates a fresh + // LifecycleInjector (Eureka client, Spectator registry, connection pools) on every call; without + // closing the old one its background threads keep it alive and it leaks, eventually exhausting the heap. + if (lifecycleManager != null) { + try { + lifecycleManager.close(); + } catch (Exception e) { + log.warn("Failed to close previous lifecycle manager during refresh", e); + } + } setupEnv(); testEVCache(); } @@ -278,6 +288,7 @@ public void functionalTestsWithAppLevelAndASGLevelHashingScenarios() throws Exce refreshEVCache(); assertTrue(manager.getEVCacheConfig().getPropertyRepository().get(appName + ".hash.key", Boolean.class).orElse(false).get()); doFunctionalTests(true); + testBulkHashed(); propertiesToSet.remove(appName + ".hash.key"); // hashing at app level due to auto hashing as a consequence of a large key @@ -331,6 +342,84 @@ public void functionalTestsWithAppLevelAndASGLevelHashingScenarios() throws Exce refreshEVCache(); } + @Test(dependsOnMethods = { "functionalTestsWithAppLevelAndASGLevelHashingScenarios" }) + public void testChunkingScenarios() throws Exception { + // chunking only (no hashing): large values are split into chunks and reassembled on read + propertiesToSet.put(appName + ".chunk.data", "true"); + refreshEVCache(); + assertTrue(manager.getEVCacheConfig().getPropertyRepository().get(appName + ".chunk.data", Boolean.class).orElse(false).get()); + doChunkingTests(false); + + // chunking + hashing together: the value is wrapped in an EVCacheValue envelope and then chunked under the + // hashed key. This exercises the mixed-key, chunk-aware getBulk (two-step decode after chunk reassembly). + propertiesToSet.put(appName + ".hash.key", "true"); + refreshEVCache(); + assertTrue(manager.getEVCacheConfig().getPropertyRepository().get(appName + ".hash.key", Boolean.class).orElse(false).get()); + doChunkingTests(true); + propertiesToSet.remove(appName + ".hash.key"); + + propertiesToSet.remove(appName + ".chunk.data"); + refreshEVCache(); + } + + private void doChunkingTests(boolean hashingEnabled) throws Exception { + final EVCacheClient client = manager.getEVCacheClientPool(appName).getEVCacheClientForRead(); + + // single large value -> chunked set/get + final String largeKey = "chunk_large_" + System.nanoTime(); + final String largeValue = buildLargeValue(4000); + EVCacheLatch latch = evCache.set(largeKey, largeValue, EVCacheLatch.Policy.ALL); + latch.await(10000, TimeUnit.MILLISECONDS); + + // verify the value was actually chunked (guards against it being too small / compressed below chunk.size). + // For hashed keys the stored key is the hash key, so we only introspect the chunk layout for plain keys. + if (!hashingEnabled) { + final Map chunks = client.getAllChunks("cid:" + largeKey); + assertNotNull(chunks, "large value should exist in cache"); + assertFalse(chunks.containsKey("cid:" + largeKey), + "value should have been chunked, but was stored as a single key (too small / compressed below chunk.size)"); + } + + assertEquals(evCache.get(largeKey), largeValue, "chunked single get did not return the written value"); + + // bulk get of multiple chunked values (sync getBulk; async bulk does not support chunking) + final int count = 3; + final Map kv = new HashMap<>(count); + for (int i = 0; i < count; i++) { + final String key = "chunk_bulk_" + i + "_" + System.nanoTime(); + final String value = buildLargeValue(3000 + i); + kv.put(key, value); + EVCacheLatch l = evCache.set(key, value, EVCacheLatch.Policy.ALL); + l.await(10000, TimeUnit.MILLISECONDS); + } + final Map results = evCache.getBulk(kv.keySet().toArray(new String[0])); + assertNotNull(results); + assertEquals(results.size(), kv.size(), "chunked getBulk returned wrong number of entries"); + for (Map.Entry entry : kv.entrySet()) { + assertEquals(results.get(entry.getKey()), entry.getValue(), "chunked getBulk failed for key " + entry.getKey()); + } + + // cleanup + for (Future f : evCache.delete(largeKey)) { + f.get(); + } + for (String key : kv.keySet()) { + for (Future f : evCache.delete(key)) { + f.get(); + } + } + } + + // Builds an incompressible value (random UUIDs) so its encoded size stays above the chunk size and actually + // triggers chunking. A repeating/low-entropy value would be compressed below chunk.size and never chunk. + private String buildLargeValue(int approxBytes) { + final StringBuilder sb = new StringBuilder(approxBytes + 36); + while (sb.length() < approxBytes) { + sb.append(UUID.randomUUID().toString()); + } + return sb.toString(); + } + private void testWithLargeKey() throws Exception { StringBuilder sb = new StringBuilder(); for (int i= 0; i < 100; i++) { @@ -354,11 +443,43 @@ private void testWithLargeKey() throws Exception { } } - private void testWithMixedKeys() throws Exception { + private void testBulkHashed() throws Exception { + final int count = 3; + Map kv = new HashMap<>(count); + for (int i = 0; i < count; i++) { + String key = "bulkhashed_" + i; + String value = "val_bulkhashed_" + i; + kv.put(key, value); + EVCacheLatch latch = evCache.set(key, value, EVCacheLatch.Policy.ALL); + latch.await(10000, TimeUnit.MILLISECONDS); + } + + Map results = evCache.getBulk(kv.keySet().toArray(new String[0])); + assertNotNull(results); + assertEquals(results.size(), kv.size()); + for (Map.Entry entry : kv.entrySet()) { + assertEquals(results.get(entry.getKey()), entry.getValue(), + "getBulk with all hashed keys failed for key " + entry.getKey()); + } + + CompletableFuture> future = evCache.getAsyncBulk(kv.keySet().toArray(new String[0])); + results = future.get(10000, TimeUnit.MILLISECONDS); + assertNotNull(results); + assertEquals(results.size(), kv.size()); + for (Map.Entry entry : kv.entrySet()) { + assertEquals(results.get(entry.getKey()), entry.getValue(), + "getAsyncBulk with all hashed keys failed for key " + entry.getKey()); + } + + for (String key : kv.keySet()) { + Future[] deleteFutures = evCache.delete(key); + for (Future deleteFuture : deleteFutures) { + deleteFuture.get(); + } + } + } - EVCache[] evcacheInstance = new EVCache[2]; - evcacheInstance[0] = getNewBuilder().setAppName(appName).setCachePrefix("cid").enableRetry().build(); - evcacheInstance[1] = this.evCache; + private void testWithMixedKeys() throws Exception { Map kv = new HashMap<>(6); String oneLargeKey = null; @@ -457,8 +578,9 @@ private void testWithMixedKeysAndCustomTranscoder() throws Exception { results = evCache.getBulk(kv.keySet().toArray(new String[0])); } + assertEquals(results.size(), kv.size()); for (Map.Entry result : results.entrySet()) { - assertEquals(results.size(), kv.size()); + assertEquals(result.getValue(), kv.get(result.getKey()), "Did not get the written value back with op " + (op == 0 ? "getAsyncBulk" : "getBulk")); } } diff --git a/evcache-core/src/main/java/com/netflix/evcache/EVCacheImpl.java b/evcache-core/src/main/java/com/netflix/evcache/EVCacheImpl.java index 19f7e42b..e0395ebf 100644 --- a/evcache-core/src/main/java/com/netflix/evcache/EVCacheImpl.java +++ b/evcache-core/src/main/java/com/netflix/evcache/EVCacheImpl.java @@ -1973,14 +1973,6 @@ private Map getBulkData(EVCacheClient client, Collection plainKeys = keyMapDto.getPlainKeysMap().keySet(); final Set hashedKeys = keyMapDto.getHashedKeysMap().keySet(); - // Preserve chunking behavior for plain-only requests: route through the existing single-transcoder - // path, which is the only one that honors enableChunking. Chunked apps do not use hashed keys. - if (hashedKeys.isEmpty()) { - if (tc == null && _transcoder != null) tc = (Transcoder) _transcoder; - final Map objMap = client.getBulk(plainKeys, tc, throwException, hasZF); - return buildKeyValueResult(objMap, keyMapDto); - } - final BiPredicate collisionChecker = (hashedKey, decodedKey) -> { final EVCacheKey evcKey = keyMapDto.getHashedKeysMap().get(hashedKey); if (evcKey.getCanonicalKey(client.isDuetClient()).equals(decodedKey)) { diff --git a/evcache-core/src/main/java/com/netflix/evcache/pool/EVCacheClient.java b/evcache-core/src/main/java/com/netflix/evcache/pool/EVCacheClient.java index cc5a1af7..83c7a6b5 100644 --- a/evcache-core/src/main/java/com/netflix/evcache/pool/EVCacheClient.java +++ b/evcache-core/src/main/java/com/netflix/evcache/pool/EVCacheClient.java @@ -34,6 +34,7 @@ import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -568,7 +569,34 @@ private ChunkInfo getChunkInfo(String firstKey, String metadata) { return ci; } - private Map assembleChunks(Collection keyList, Transcoder tc, boolean hasZF) { + private T decodeForKey(String key, CachedData raw, Collection plainKeys, Set hashedKeys, Transcoder valueTranscoder, EVCacheTranscoder evcacheValueTranscoder, BiPredicate collisionChecker, boolean hasZF) { + if (raw == null) return null; + // hashed keys require 2 step decoding, first using envelopeTranscoder then using valueTranscoder + if (hashedKeys != null && hashedKeys.contains(key)) { + if (evcacheValueTranscoder == null) throw new IllegalStateException("Both transcoders required for 2-step decode, failed on key " + key + + " of bulk get for plain keys [" + plainKeys + "] and hashed keys [" + hashedKeys + "]"); + Object obj; + try { + obj = evcacheValueTranscoder.decode(raw); + } catch (Exception e) { + throw new RuntimeException("Failed to decode key " + key + " using envelopeTranscoder " + evcacheValueTranscoder.getClass().getName(), e); + } + if (obj instanceof EVCacheValue) { + final EVCacheValue val = (EVCacheValue) obj; + boolean collision = collisionChecker.test(key, val.getKey()); + if (!collision) { + return valueTranscoder.decode(new CachedData(val.getFlags(), val.getValue(), valueTranscoder.getMaxSize())); + } + } + return null; + } + return valueTranscoder.decode(raw); + } + + private Map assembleChunks(Collection plainKeys, Set hashedKeys, Transcoder valueTranscoder, EVCacheTranscoder evcacheValueTranscoder, BiPredicate collisionChecker, boolean hasZF) { + final Set keyList = new HashSet<>(); + if (plainKeys != null) keyList.addAll(plainKeys); + if (hashedKeys != null) keyList.addAll(hashedKeys); final List firstKeys = new ArrayList<>(); for (String key : keyList) { firstKeys.add(key); @@ -583,7 +611,7 @@ private Map assembleChunks(Collection keyList, Transcoder for (String key : keyList) { if (metadataMap.containsKey(key)) { CachedData val = metadataMap.remove(key); - returnMap.put(key, tc.decode(val)); + returnMap.put(key, decodeForKey(key, val, plainKeys, hashedKeys, valueTranscoder, evcacheValueTranscoder, collisionChecker, hasZF)); } } @@ -652,7 +680,7 @@ private Map assembleChunks(Collection keyList, Transcoder final boolean checksumPass = checkCRCChecksum(data, ci, hasZF); if (data != null && checksumPass) { final CachedData cd = new CachedData(ci.getFlags(), data, Integer.MAX_VALUE); - returnMap.put(ci.getKey(), tc.decode(cd)); + returnMap.put(ci.getKey(), decodeForKey(ci.getKey(), cd, plainKeys, hashedKeys, valueTranscoder, evcacheValueTranscoder, collisionChecker, hasZF)); } else { returnMap.put(ci.getKey(), null); } @@ -664,6 +692,14 @@ private Map assembleChunks(Collection keyList, Transcoder return null; } + /** + * Plain-only chunk assembly. Delegates to the mixed-key variant with an empty hashed-key set, so every key is + * decoded in a single step (identical to the legacy single-transcoder behavior). + */ + private Map assembleChunks(Collection keyList, Transcoder tc, boolean hasZF) { + return assembleChunks(keyList, Collections.emptySet(), tc, null, null, hasZF); + } + private Single> assembleChunks(Collection keyList, Transcoder tc, boolean hasZF, Scheduler scheduler) { final List firstKeys = new ArrayList<>(); for (String key : keyList) { @@ -973,6 +1009,12 @@ public Single getAndTouch(String key, Transcoder transcoder, int timeT } } + /** + * @deprecated Does not support a mix of plain and hashed keys. Use + * {@link #getBulk(Collection, Set, Transcoder, EVCacheTranscoder, String, boolean, BiPredicate, boolean, boolean)}, + * which handles plain and hashed keys (and chunking) in a single request. + */ + @Deprecated public Map getBulk(Collection canonicalKeys, Transcoder tc, boolean _throwException, boolean hasZF) throws Exception { final Map returnVal; @@ -999,11 +1041,11 @@ public Map getBulk(Collection canonicalKeys, Transcoder Map getBulk(Collection plainKeys, Set hashedKeys, Transcoder valueTranscoder, EVCacheTranscoder evcacheValueTranscoder, @@ -1011,6 +1053,9 @@ public Map getBulk(Collection plainKeys, Set hash boolean _throwException, boolean hasZF) throws Exception { try { if (valueTranscoder == null) valueTranscoder = (Transcoder) getTranscoder(); + if (enableChunking.get()) { + return assembleChunks(plainKeys, hashedKeys, valueTranscoder, evcacheValueTranscoder, collisionChecker, hasZF); + } final BiPredicate validator = (node, key) -> { NodeValidationResult result = validateNodeForRead(node, Call.BULK, 2 * maxReadQueueSize.get()); if (result != NodeValidationResult.OK) { From d3916b1861e9ac43f618e38af39a022871c47fe1 Mon Sep 17 00:00:00 2001 From: Bihao Xu Date: Fri, 5 Jun 2026 10:28:15 -0700 Subject: [PATCH 3/4] fix: drop collided keys and avoid NPE in chunked getBulk --- .../netflix/evcache/pool/EVCacheClient.java | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/evcache-core/src/main/java/com/netflix/evcache/pool/EVCacheClient.java b/evcache-core/src/main/java/com/netflix/evcache/pool/EVCacheClient.java index 83c7a6b5..11a261bd 100644 --- a/evcache-core/src/main/java/com/netflix/evcache/pool/EVCacheClient.java +++ b/evcache-core/src/main/java/com/netflix/evcache/pool/EVCacheClient.java @@ -593,7 +593,7 @@ private T decodeForKey(String key, CachedData raw, Collection plainK return valueTranscoder.decode(raw); } - private Map assembleChunks(Collection plainKeys, Set hashedKeys, Transcoder valueTranscoder, EVCacheTranscoder evcacheValueTranscoder, BiPredicate collisionChecker, boolean hasZF) { + private Map assembleChunks(Collection plainKeys, Set hashedKeys, Transcoder valueTranscoder, EVCacheTranscoder evcacheValueTranscoder, BiPredicate collisionChecker, boolean hasZF) throws Exception { final Set keyList = new HashSet<>(); if (plainKeys != null) keyList.addAll(plainKeys); if (hashedKeys != null) keyList.addAll(hashedKeys); @@ -611,7 +611,10 @@ private Map assembleChunks(Collection plainKeys, Set Map assembleChunks(Collection plainKeys, Set Map assembleChunks(Collection plainKeys, Set Map assembleChunks(Collection keyList, Transcoder tc, boolean hasZF) { + private Map assembleChunks(Collection keyList, Transcoder tc, boolean hasZF) throws Exception { return assembleChunks(keyList, Collections.emptySet(), tc, null, null, hasZF); } @@ -1054,7 +1060,8 @@ public Map getBulk(Collection plainKeys, Set hash try { if (valueTranscoder == null) valueTranscoder = (Transcoder) getTranscoder(); if (enableChunking.get()) { - return assembleChunks(plainKeys, hashedKeys, valueTranscoder, evcacheValueTranscoder, collisionChecker, hasZF); + final Map chunked = assembleChunks(plainKeys, hashedKeys, valueTranscoder, evcacheValueTranscoder, collisionChecker, hasZF); + return chunked == null ? Collections.emptyMap() : chunked; } final BiPredicate validator = (node, key) -> { NodeValidationResult result = validateNodeForRead(node, Call.BULK, 2 * maxReadQueueSize.get()); From 79d1d730c72a2caf9a4e16c6ba62dc92bc506604 Mon Sep 17 00:00:00 2001 From: Bihao Xu Date: Mon, 8 Jun 2026 14:18:43 -0700 Subject: [PATCH 4/4] Add test for mixed-key chunked getBulk (plain/hashed x chunked/non-chunked) --- .../netflix/evcache/test/EVCacheTestDI.java | 101 ++++++++++++++---- .../java/com/netflix/evcache/EVCacheImpl.java | 3 +- .../netflix/evcache/pool/EVCacheClient.java | 9 +- 3 files changed, 91 insertions(+), 22 deletions(-) diff --git a/evcache-client/test/com/netflix/evcache/test/EVCacheTestDI.java b/evcache-client/test/com/netflix/evcache/test/EVCacheTestDI.java index e1e4fbb8..46cd2d84 100644 --- a/evcache-client/test/com/netflix/evcache/test/EVCacheTestDI.java +++ b/evcache-client/test/com/netflix/evcache/test/EVCacheTestDI.java @@ -261,9 +261,9 @@ public void testAppendOrAdd() throws Exception { } private void refreshEVCache() { - // Close the previous DI container before building a new one. setupEnv() creates a fresh - // LifecycleInjector (Eureka client, Spectator registry, connection pools) on every call; without - // closing the old one its background threads keep it alive and it leaks, eventually exhausting the heap. + // Close the previous DI container before building a new one. setupEnv() builds a fresh LifecycleInjector on + // every call; without closing the old ones they accumulate across refreshes and the suite slows to a crawl, + // to the point it cannot finish. if (lifecycleManager != null) { try { lifecycleManager.close(); @@ -348,21 +348,22 @@ public void testChunkingScenarios() throws Exception { propertiesToSet.put(appName + ".chunk.data", "true"); refreshEVCache(); assertTrue(manager.getEVCacheConfig().getPropertyRepository().get(appName + ".chunk.data", Boolean.class).orElse(false).get()); - doChunkingTests(false); + doChunkingTests(); - // chunking + hashing together: the value is wrapped in an EVCacheValue envelope and then chunked under the - // hashed key. This exercises the mixed-key, chunk-aware getBulk (two-step decode after chunk reassembly). - propertiesToSet.put(appName + ".hash.key", "true"); + // chunking + auto-hashing together: with auto.hash.keys, short keys stay plain while keys whose canonical form + // exceeds max.key.length are hashed. A single getBulk over both exercises the mixed-key, chunk-aware path: plain + // keys decode in one step, hashed keys are EVCacheValue-wrapped and decode in two steps, all after reassembly. + propertiesToSet.put(appName + ".auto.hash.keys", "true"); refreshEVCache(); - assertTrue(manager.getEVCacheConfig().getPropertyRepository().get(appName + ".hash.key", Boolean.class).orElse(false).get()); - doChunkingTests(true); - propertiesToSet.remove(appName + ".hash.key"); + assertTrue(manager.getEVCacheConfig().getPropertyRepository().get(appName + ".auto.hash.keys", Boolean.class).orElse(false).get()); + doMixedKeyChunkingTests(); + propertiesToSet.remove(appName + ".auto.hash.keys"); propertiesToSet.remove(appName + ".chunk.data"); refreshEVCache(); } - private void doChunkingTests(boolean hashingEnabled) throws Exception { + private void doChunkingTests() throws Exception { final EVCacheClient client = manager.getEVCacheClientPool(appName).getEVCacheClientForRead(); // single large value -> chunked set/get @@ -371,14 +372,8 @@ private void doChunkingTests(boolean hashingEnabled) throws Exception { EVCacheLatch latch = evCache.set(largeKey, largeValue, EVCacheLatch.Policy.ALL); latch.await(10000, TimeUnit.MILLISECONDS); - // verify the value was actually chunked (guards against it being too small / compressed below chunk.size). - // For hashed keys the stored key is the hash key, so we only introspect the chunk layout for plain keys. - if (!hashingEnabled) { - final Map chunks = client.getAllChunks("cid:" + largeKey); - assertNotNull(chunks, "large value should exist in cache"); - assertFalse(chunks.containsKey("cid:" + largeKey), - "value should have been chunked, but was stored as a single key (too small / compressed below chunk.size)"); - } + // verify the value was actually chunked (guards against it being too small / compressed below chunk.size) + assertChunked(client, "cid:" + largeKey); assertEquals(evCache.get(largeKey), largeValue, "chunked single get did not return the written value"); @@ -410,6 +405,74 @@ private void doChunkingTests(boolean hashingEnabled) throws Exception { } } + // Exercises chunking together with a real mixed-key bulk request. Requires auto.hash.keys=true: short keys whose + // canonical form ("cid:"+key) stays within max.key.length remain plain, while long keys that exceed it are hashed. + // Each group has a large (chunked) and a small (stored directly, below chunk.size) value, so the single getBulk + // drives all four combinations: plain/hashed x chunked/non-chunked. Plain decodes in one step, hashed in two. + private void doMixedKeyChunkingTests() throws Exception { + final EVCacheClient client = manager.getEVCacheClientPool(appName).getEVCacheClientForRead(); + + final Map kv = new HashMap<>(); + + // short keys: canonical form stays under max.key.length (default 200) -> remain plain + final String plainChunkedKey = "chunked_plain_" + System.nanoTime(); + kv.put(plainChunkedKey, buildLargeValue(3000)); + final String plainNonChunkedKey = "nonchunked_plain_" + System.nanoTime(); + kv.put(plainNonChunkedKey, UUID.randomUUID().toString()); + + // long keys: canonical form exceeds max.key.length -> auto-hashed (buildLargeValue(220) guarantees > 200) + final String hashedChunkedKey = "chunked_hashed_" + buildLargeValue(220); + kv.put(hashedChunkedKey, buildLargeValue(3000)); + final String hashedNonChunkedKey = "nonchunked_hashed_" + buildLargeValue(220); + kv.put(hashedNonChunkedKey, UUID.randomUUID().toString()); + + for (Map.Entry entry : kv.entrySet()) { + evCache.set(entry.getKey(), entry.getValue(), EVCacheLatch.Policy.ALL).await(10000, TimeUnit.MILLISECONDS); + } + + // structural proof of the storage layout on the plain path (stored verbatim, no hashing, so introspectable): + // the large value is split into chunks, the small value is stored under a single key. The hashed equivalents use + // the same value sizes and their correct round-trip below confirms the hashed write/reassembly path. + assertChunked(client, "cid:" + plainChunkedKey); + assertNotChunked(client, "cid:" + plainNonChunkedKey); + + // mixed-key, chunk-aware getBulk: plain keys decode in one step, hashed keys in two steps; chunked values are + // reassembled first, non-chunked values are decoded directly. + final Map results = evCache.getBulk(kv.keySet().toArray(new String[0])); + assertNotNull(results); + assertEquals(results.size(), kv.size(), "mixed-key chunked getBulk returned wrong number of entries"); + for (Map.Entry entry : kv.entrySet()) { + assertEquals(results.get(entry.getKey()), entry.getValue(), "mixed-key chunked getBulk failed for key " + entry.getKey()); + } + + // single get round-trip for hashed (two-step decode) keys, both chunked and non-chunked + assertEquals(evCache.get(hashedChunkedKey), kv.get(hashedChunkedKey), "chunked single get of hashed key failed"); + assertEquals(evCache.get(hashedNonChunkedKey), kv.get(hashedNonChunkedKey), "non-chunked single get of hashed key failed"); + + // cleanup + for (String key : kv.keySet()) { + for (Future f : evCache.delete(key)) { + f.get(); + } + } + } + + // getAllChunks returns the chunk keys (_01, _02, ...) when the value was chunked, or a single entry + // keyed by storageKey itself when stored unchunked. So absence of storageKey in the returned map proves chunking. + private void assertChunked(EVCacheClient client, String storageKey) throws Exception { + final Map chunks = client.getAllChunks(storageKey); + assertNotNull(chunks, "large value should exist in cache for key " + storageKey); + assertFalse(chunks.containsKey(storageKey), + "value should have been chunked, but was stored as a single key (too small / compressed below chunk.size): " + storageKey); + } + + // Inverse of assertChunked: a non-chunked value is returned as a single entry keyed by storageKey itself. + private void assertNotChunked(EVCacheClient client, String storageKey) throws Exception { + final Map chunks = client.getAllChunks(storageKey); + assertNotNull(chunks, "small value should exist in cache for key " + storageKey); + assertTrue(chunks.containsKey(storageKey), "value should have been stored as a single (non-chunked) key: " + storageKey); + } + // Builds an incompressible value (random UUIDs) so its encoded size stays above the chunk size and actually // triggers chunking. A repeating/low-entropy value would be compressed below chunk.size and never chunk. private String buildLargeValue(int approxBytes) { diff --git a/evcache-core/src/main/java/com/netflix/evcache/EVCacheImpl.java b/evcache-core/src/main/java/com/netflix/evcache/EVCacheImpl.java index e0395ebf..cefa384d 100644 --- a/evcache-core/src/main/java/com/netflix/evcache/EVCacheImpl.java +++ b/evcache-core/src/main/java/com/netflix/evcache/EVCacheImpl.java @@ -1989,7 +1989,8 @@ private Map getBulkData(EVCacheClient client, Collection valueTranscoder = (tc == null) ? ((_transcoder == null) ? (Transcoder) client.getTranscoder() : (Transcoder) _transcoder) : tc; if (log.isDebugEnabled() && shouldLog()) - log.debug("fetching bulk data with set of keys containing hashed key(s) {} ", evcacheKeys); + log.debug("fetching bulk data for APP " + _appName + " with " + plainKeys.size() + " plain and " + + hashedKeys.size() + " hashed key(s) : {}", evcacheKeys); final Map objMap = client.getBulk(plainKeys, hashedKeys, valueTranscoder, evcacheValueTranscoder, _appName, shouldLog(), collisionChecker, throwException, hasZF); return buildKeyValueResult(objMap, keyMapDto); diff --git a/evcache-core/src/main/java/com/netflix/evcache/pool/EVCacheClient.java b/evcache-core/src/main/java/com/netflix/evcache/pool/EVCacheClient.java index 11a261bd..462f83c3 100644 --- a/evcache-core/src/main/java/com/netflix/evcache/pool/EVCacheClient.java +++ b/evcache-core/src/main/java/com/netflix/evcache/pool/EVCacheClient.java @@ -587,6 +587,11 @@ private T decodeForKey(String key, CachedData raw, Collection plainK if (!collision) { return valueTranscoder.decode(new CachedData(val.getFlags(), val.getValue(), valueTranscoder.getMaxSize())); } + } else if (log.isDebugEnabled()) { + // Mirrors the non-chunked path in EVCacheMemcachedClient.asyncGetBulk: decoding a hashed key did not yield + // an EVCacheValue (e.g. a hashed/raw key collision). The key is dropped from the result below. + log.debug("APP " + appName + ", applying envelopeTranscoder to hashed key " + key + + " did not yield an EVCacheValue (possible collision); dropping from result"); } return null; } @@ -699,8 +704,8 @@ private Map assembleChunks(Collection plainKeys, Set Map assembleChunks(Collection keyList, Transcoder tc, boolean hasZF) throws Exception { return assembleChunks(keyList, Collections.emptySet(), tc, null, null, hasZF);