Add Zstd support for EVCache#197
Conversation
|
@shy-1234 can you also review the PR please |
| public Transcoder<Object> getDefaultTranscoder() { | ||
| return new EVCacheTranscoder(); | ||
| return new EVCacheTranscoder(appName, | ||
| client.getPool().getEVCacheClientPoolManager().getEVCacheConfig().getPropertyRepository()); |
There was a problem hiding this comment.
nit: why not just appName? seems like there is a constructor that handles this.
There was a problem hiding this comment.
the constructor without PropertyRepository config argumnt uses EVCacheConfig.getInstance().getPropertyRepository(), which is a global setting. If an application uses multiple evcaches, they all share the same config, which is NOT optimal, but maybe ok for default.evcache.max.data.size. Ideally, we should have different config for different evache as each may have different requirement. per cache FP should use client.getPool().getEVCacheClientPoolManager().getEVCacheConfig().getPropertyRepository().
For backward compatibility, we should not change default.evcache.max.data.size.
There was a problem hiding this comment.
I feel like the statement is not correct, but please correct me if I am wrong.
First of all
EVCacheConfig.propertyRepository is a static field (line 27). There is only ever one value for the entire JVM, regardless of how many EVCacheConfig instances exist.
EVCacheConfig is also a singleton with a single INSTANCE.
Following the longer path:
- client.getPool().getEVCacheClientPoolManager() returns the pool manager
- .getEVCacheConfig() returns this.evcConfig — which is set from line 153: new EVCacheClientPoolManager(..., EVCacheConfig.getInstance())
- .getPropertyRepository() returns the static propertyRepository field
Both paths terminate at the exact same static field. They return the same object.
Secondly,
If an application uses multiple evcaches, they all share the same config, which is NOT optimal
Agree, but IIUC the way the client code achieves per cluster config is by appending evcache cluster name in front of the FP and fallback to the global one (prefix with "evcache") if not provided. (not saying it's good or bad though, but in this case, if your intention is to achieve per cluster config, then i don't think it does the work.)
| if (in == null) throw new NullPointerException("Can't compress null"); | ||
|
|
||
| CompressionAlgorithm compressionAlgorithm = compressionAlgorithmProperty == null ? CompressionAlgorithm.GZIP | ||
| : CompressionAlgorithm.valueOf(compressionAlgorithmProperty.orElse(CompressionAlgorithm.GZIP.name()).get().toUpperCase()); |
There was a problem hiding this comment.
Must the choice of algorithm and the level be dynamic? Could this be something we set at evcache client creation time? This would mean you would need to restart the server to pick up a client change.
| /** | ||
| * Transcoder that serializes and compresses objects. | ||
| */ | ||
| public class EVCacheSerializingTranscoder extends BaseSerializingTranscoder implements |
There was a problem hiding this comment.
@janewang1680
Can we move the compression changes from EVCacheSerializingTranscoder down it's subclass EVCacheTranscoder.
Benefits:
- Isolates most of the compression logic to a single transcoder file.
- Removes the need for setter methods, default property constants, and null checks
(compressionAlgorithmProperty == null ? ..., - Keeps
EVCacheSerializingTranscoderas a generic serialize base class.
| public EVCacheTranscoder(String appName, PropertyRepository config, int max, int compressionThreshold) { | ||
| super(appName, max); | ||
| setCompressionThreshold(compressionThreshold); | ||
| Property<String> algoProperty = getProperty(config, "evcacheclient.compression.algo", String.class); |
There was a problem hiding this comment.
Can we name this so the naming convention is uniform?
default.evcache.compression.algo
| setCompressionThreshold(compressionThreshold); | ||
| Property<String> algoProperty = getProperty(config, "evcacheclient.compression.algo", String.class); | ||
| setCompressionAlgorithmProperty(algoProperty); | ||
| Property<Integer> zstdLevelProperty = getProperty(config, "evcacheclient.compression.zstd.level", Integer.class); |
There was a problem hiding this comment.
Same here
default.evcache.compression.zstd.level
4 participants
Summary
Add pluggable compression algorithm support (gzip, zstd) via a new CompressionAlgorithm enum on EVCacheSerializingTranscoder, with magic-byte auto-detection on decode so existing gzip-compressed cache entries
continue to decode correctly. Algorithm and zstd level are configurable via two new fast properties:
default.evcache.compression.algorithm (default GZIP)
default.evcache.compression.zstd.level (default 3)
Fix a pre-existing bug in the compression ratio metric where compressed.length / b.length always evaluated to 1.0 because b was reassigned to compressed before the ratio calculation. The metric now uses the
captured originalLength, producing accurate ratios.
Test plan
./gradlew :evcache-core:test --tests "com.netflix.evcache.EVCacheSerializingTranscoderTest" — 17 tests covering enum values, default constructor, gzip/zstd round-trip, cross-decode (gzip-written read by zstd
transcoder and vice versa), and FP wiring.
Verify backward compatibility: data written with gzip in production decodes correctly after deploy.
Verify compression ratio metric (COMPRESSION_RATIO timer) reports realistic ratios (not always ~100%) in a canary.
Verify it works in our internal services