Conversation
messageDocsToJsonSchema walks every message type's full field tree via Scala runtime reflection (recursive <:</=:= subtype checks) to build the schema, with no caching anywhere in this file. The caller in Http4s600 does have a Redis-backed cache in front of it, but that one silently falls through to a full recompute whenever Redis is unreachable or slow -- which is exactly what happens once the reflection cost itself starts causing GC pressure, turning this into a positive feedback loop under sustained polling. For a given connector the message docs, and therefore the schema, are static for the life of the JVM, so memoize the result in-process (Guava-backed, no Redis dependency) keyed by connector name, using the same memoizeSyncWithImMemory + CacheKeyFromArguments pattern already proven out by Helper.getRequiredFieldInfo. This is a second, independent line of defense alongside the existing Redis-backed cache, not a replacement for it. Verified against the existing MessageDocsJsonSchemaTest suite (8/8 scenarios passing across rabbitmq/rest/akka connectors, including draft-07 schema validation), confirming the cached path returns identical output.
MessageDocsJsonSchemaTest already covers output correctness end to end via HTTP, but a broken cache (wrong key derivation, wrong TTL handling) could still pass a correctness-only check by silently recomputing every time -- which is exactly what happened once already while writing this fix, caught only by the compiler rejecting a cache-key placeholder with the wrong tuple arity. This adds a direct, non-HTTP test of the cache itself: identical output on a hit, a measurable speedup on a hit (median of several warm calls against a cold call, sized against a case-class tree wide/deep enough that the reflection cost isn't lost in JIT/GC noise), and that different connector names get independent cache entries.
|
4 tasks
Owner
Author
|
Superseded — squashed together with #102 and #104 into one combined commit, PR'd upstream directly: OpenBankProject#2920 |
1 task
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
1 participant
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
JsonSchemaGenerator.messageDocsToJsonSchemawalks every message type's field tree via Scala runtime reflection on every call, with no cache anywhere in the file -- pure waste, since the schema is static per connector for the life of the JVM. The caller's Redis-backed cache (Http4s600) also silently falls through to a full recompute on any Redis GET failure, so this path has no cache at all when Redis is having a bad day.Not the production incident's cause (see #102):
JsonSchemaGeneratordoesn't appear in that incident's JFR samples, and there were zero Redis failures in the log window. This PR is independent hardening for a real gap in this file, not a fix for that incident -- an earlier version of this description claimed otherwise.Change
Memoize the result in-process, keyed by connector name, using the same
memoizeSyncWithImMemory+CacheKeyFromArgumentspattern already proven correct byHelper.getRequiredFieldInfo. Guava-backed, no Redis dependency -- a second, independent cache alongside the existing Redis one, not a replacement.Test plan
MessageDocsJsonSchemaTest(8/8) -- identical output with caching enabledJsonSchemaGeneratorCacheTest(3/3) -- cache-hit output matches, measurable speedup, per-connector isolationSuperseded -- squashed together with #102 and #104 into one combined commit, PR'd upstream directly: OpenBankProject#2920