Move binary serialization to EVCacheSerializingTranscoder Refactor#200
Open
joegoogle123 wants to merge 1 commit into
Open
Move binary serialization to EVCacheSerializingTranscoder Refactor#200joegoogle123 wants to merge 1 commit into
joegoogle123 wants to merge 1 commit into
Conversation
Refactor PR #196's binary-serde plumbing so the EVCacheValue dispatch lives on the superclass and callers get typed accessors instead of the generic getProperty escape hatch. - Move the serialize/deserialize overrides that gate on isBinarySerializationEnabled + EVCacheValueSerde.isBinaryFormat from EVCacheTranscoder up to EVCacheSerializingTranscoder. The subclass is now a thin wrapper: only encode() still overrides super to pass CachedData through unchanged. - Drop the trivial asyncDecode/decode overrides on EVCacheTranscoder that only delegated to super. - Reorder the EVCacheSerializingTranscoder constructor to (int max, EVCacheTranscoderProperties). Rename the properties field to transcoderProperties. Subclasses that need appName read it from transcoderProperties.getAppName() rather than a dedicated field. - On EVCacheTranscoderProperties, snapshot maxDataSizeBytes and compressionThresholdBytes as primitives at construction and expose them via typed getters (getMaxDataSizeBytes, getCompressionThresholdBytes, getAppName). Drop the public getProperty(Key, Class, T) escape hatch; no callers remain now that the transcoder chain uses the typed accessors. Dynamic fields will be exposed as Property<T> accessors. - Wire the connection factories' getDefaultTranscoder() through a per-app EVCacheTranscoderProperties so the per-app FP prefix reaches the default transcoder path — PR #196 wired this for the value transcoder but missed the default transcoder built here. - Delete unused imports (ServerGroup, StringUtils, Map, ConcurrentHashMap) left behind on EVCacheSerializingTranscoder. Test coverage in EVCacheTranscoderPropertiesTest is migrated to the typed getters and extended with two cases for getAppName. Existing resolution-chain cases (per-app → global → static default) are preserved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
joegoogle123
force-pushed
the
joelee/refactor-move-binary-serde
branch
from
e72af3f to
45d17db
Compare
Contributor
Author
|
@janewang1680 once this merges you can rebase off of this for your compression PR. |
shy-1234
reviewed
Jul 8, 2026
| * <p> | ||
| * Static properties should be cached as a field for fast access. | ||
| * Dynamic properties get be accessed {@link #getProperty(Key, Class, Object)} | ||
| * Static properties are snapshotted as primitive fields for fast access. Dynamic fields |
Contributor
There was a problem hiding this comment.
Not a blocker since it's the original behavior, but just curious how much overhead would it be to also make those static properties dynamic? (I think the dynamic values are also cached?)
Contributor
Author
There was a problem hiding this comment.
There are two fields that where static e.g max and compressionThreshold which I don't think we can make dynamic.
There is a new field to binarySerializationEnabled which we could make dynamic but I did not measure the overhead of dynamically checking this property.
shy-1234
approved these changes
Jul 8, 2026
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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
Follow-up refactor to PR #196. Moves the
EVCacheValuebinary serde dispatch onto the superclass and cleans up the transcoder properties API so callers get typed accessors instead of the genericgetProperty(Key, Class, T)escape hatch.Zero behavior change on the wire — the same
serialize/deserializegating andEVCacheValueSerde.isBinaryFormatcheck just live one class up. This lands ahead of the zstd compression PR (#197) so that PR shows only compression content.What changed
EVCacheSerializingTranscoder— addedprotectedserialize/deserializeoverrides that gate ontranscoderProperties.isBinarySerializationEnabled() && o instanceof EVCacheValue(moved fromEVCacheTranscoder). Constructor reordered to(int max, EVCacheTranscoderProperties). Newprotected final String appNamefield pulled fromproperties.getAppName()for downstream metric tagging. Renamedproperties→transcoderProperties. Removed unused imports (ServerGroup,StringUtils,Map,ConcurrentHashMap).EVCacheTranscoder— now a thin wrapper: onlyencode()overrides super (to passCachedDatathrough unchanged). Removed the redundantasyncDecode/decodeoverrides that only delegated. Constructors updated to use typed getters and the new super signature.EVCacheTranscoderProperties— snapshottedmaxDataSizeBytesandcompressionThresholdBytesasfinal intat construction. Added typed gettersgetAppName(),getMaxDataSizeBytes(),getCompressionThresholdBytes(). Removed the public<T> Property<T> getProperty(Key, Class, T)method — no callers remain now that the transcoder chain uses the typed accessors.BaseAsciiConnectionFactory/BaseConnectionFactory—getDefaultTranscoder()now passes a per-appEVCacheTranscoderPropertiesbundle so the<appName>.<key>prefix reaches the default transcoder path. PR EVCacheValue: opt-in compact binary serialization with backwards-compatible reads #196 wired this for the value transcoder inEVCacheImplbut missed the default transcoder built here.EVCacheTranscoderPropertiesTest— 15 existing resolution-chain cases migrated to the typed getters. Added 2 cases forgetAppName().Test plan
./gradlew :evcache-core:test --tests "com.netflix.evcache.config.EVCacheTranscoderPropertiesTest"— 17/17 pass./gradlew :evcache-core:test --tests "com.netflix.evcache.pool.EVCacheValueSerdeTest"— 17/17 pass (dispatch coverage from PR EVCacheValue: opt-in compact binary serialization with backwards-compatible reads #196 still green since the dispatch just moved up a class)./gradlew :evcache-core:build— clean, no unused-import warnings🤖 Generated with Claude Code