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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use ConcurrentLinkedHashMap as backing for LRUMap #3531
use ConcurrentLinkedHashMap as backing for LRUMap #3531
Changes from 8 commits
dd7a597
cd4e033
47e9ae3
8befa70
8f1bdcd
0ae71ae
7d901fb
111f3b3
bfc5302
3882f17
b2c9b08
0c01d2b
674a407
53c3205
f246b9c
bac364a
517f432
c57d7e1
6936cd9
669a562
f145f0a
946549a
47a1bb1
8463041
64bd51f
f916019
1696ac0
8d29de8
a5581f7
8e6897d
6a5a5b2
4a26b39
2e132ca
dfcf976
fc6fa05
6820db2
ff39d5f
b438091
f85c808
3bc8acd
a0e8839
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps update the
LRUMap
javadoc?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly not a big deal, but the idea here was that JDK serialization would retain size settings, if possible.
But perhaps CLHM does not expose these settings after construction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
long capacity()
andsetCapacity(long)
expose those if helpful. CLHM is serializable where it retains the entries (to mirrorConcurrentHashMap
) and discards the eviction order. That differs in Guava/Caffeine, fwiw, which don't keep the entries and just the configuration.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hit issues with the old _jdkSerializeMaxEntries values - after my initial changes, the cloned LRUMap was being constructed with _jdkSerializeMaxEntries set to 0 and this meant that CLHM immediately evicted all new entries.
@cowtowncoder would it be ok to allow the backing CLHM be non-transient, then we wouldn't need any special serialization logic? As @ben-manes highlights, the eviction order would be lost but that isn't too bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think cached content serialization should not be attempted, since contents are not necessarily serializable -- and at least for serializers, deserializers, are not. Alternatively if we really want to retain contents in some cases, could make it configurable. But at this point I think I'd prefer just wiping out contents.
I think losing some of the settings is probably fine: ability to JDK serialize Jackson components is bit of an odd feature in general. Although, to be honest, I think the main important thing there is to retain configuration: as far as I know, this is relied upon by frameworks like Spark etc where workers are being sent serialized set up of handlers.