fix: drop CallContext from the endpoint-mapping and FX rate cache keys - #92
Open
hongwei1 wants to merge 1 commit into
Open
fix: drop CallContext from the endpoint-mapping and FX rate cache keys#92hongwei1 wants to merge 1 commit into
hongwei1 wants to merge 1 commit into
Conversation
CacheKeyFromArguments renders every parameter that is not annotated
@CacheKeyOmit. CallContext carries per-request state (startTime,
correlationId, url, verb, ipAddress, user), so both keys were unique per
request: the cache could never hit, and getCurrentFxRateCached wrote a fresh
Redis entry per call that lived out its TTL.
getEndpointMappings additionally cached the (mappings, callContext) tuple.
chill/Kryo cannot encode the lambda reachable through
CallContext.resourceDocument, so every write failed and cachePut swallowed it
as "result served uncached" - endpointMapping.cache.ttl.seconds bought nothing
but a WARN per call. A hit would also have handed the caller the originating
request's CallContext.
Split the memoized half into getEndpointMappingsCached(bankId) rather than
annotating callContext on the caller: CacheKeyFromArguments reads the
parameters of the method whose body ends in buildCacheKey, so binding the
result to a val first leaves it with no parameters and it emits
Nil.mkString("_") - an empty argument segment, i.e. every bankId sharing one
entry. Verified with javap that the key now renders bankId :: Nil, and that
getCurrentFxRateCached renders bankId :: from :: to :: Nil.
Add invalidateEndpointMappingCache() on create/update/delete, mirroring
invalidateMethodRoutingCache: while callContext was in the key nothing could
hit, so a stale entry was unreachable by construction; now that the cache
works, writes have to publish themselves.
|
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
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.



Problem
Two memoize keys rendered the whole
CallContext:NewStyle.function.getEndpointMappingsLocalMappedConnectorInternal.getCurrentFxRateCachedCacheKeyFromArgumentsrenders every parameter that is not annotated@CacheKeyOmit, andCallContextcarries per-request state (startTime,correlationId,url,verb,ipAddress,user). Both keys were therefore unique per request: the cache could never hit.Measured A/B with both TTLs forced on — two calls differing only in
CallContext:getCurrentFxRateCachedgetEndpointMappingsgetEndpointMappingswrote nothing because it cached the(mappings, callContext)tuple, and chill/Kryo cannot encode the lambda reachable throughCallContext.resourceDocument(KryoException: Could not serialize lambda).cachePutswallows that asresult served uncached, soendpointMapping.cache.ttl.secondsbought nothing but a WARN per call. A hit would also have handed the caller the originating request'sCallContext.This is pre-existing, not a regression. Every other cache site in the codebase already keys on business arguments only, and
ConnectorBuilderUtilstamps@CacheKeyOmitontocallContextfor the methods it generates — these two hand-written sites simply never did.Changes
@CacheKeyOmit callContextongetCurrentFxRateCached.getEndpointMappings: split the memoized half intogetEndpointMappingsCached(bankId), which caches the mappings alone and pairs them with the caller's owncallContexton return.invalidateEndpointMappingCache()on create/update/delete, mirroringinvalidateMethodRoutingCache. WhilecallContextwas in the key nothing could hit, so a stale entry was unreachable by construction; now that the cache works, writes have to publish themselves.Why a helper method rather than
@CacheKeyOmiton the callerCacheKeyFromArgumentsreads the parameters of the method whose body ends inbuildCacheKey. Binding the result to a val first —— leaves the macro with no parameters and it emits
Nil.mkString("_"): an empty argument segment, i.e. everybankIdsharing one cache entry. That compiles, and no test catches it, because both TTLs default to0andCaching.memoizeSyncWithProvidershort-circuits onDuration.Zerowithout touching Redis.Verification
javapon the compiled classes, on both Scala versions this commit has to work on — 2.12.21 (where it was developed) and 2.13.18 (this PR's base):getEndpointMappingsCached$colon$colon(aload_1, Nil)=bankId :: NilgetCurrentFxRateCached$colon$colon(aload_1, aload_2, aload_3, Nil)=bankId :: from :: to :: Nil; thecallContextslot is absentByte-for-byte the same shape on both compilers, so the macro's
@CacheKeyOmithandling and its tail-expression rule behave identically across the 2.13 migration.Testing
run_tests_parallel.sh: 3450 tests, 0 failures, 0 errors, 0 skipped — all 4 shards green.Additionally, with
endpointMapping.cache.ttl.secondsandcode.fx.exchangeRate.cache.ttl.secondsforced to 60,EndpointMappingTest,EndpointMappingBankLevelTestandExchangeRateTestpass — the caching path itself is exercised, not just short-circuited.