[RC] Use Guava instead of Caffeine caches for Android #2483
Conversation
This file contains 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
carterkozak
reviewed
Jan 30, 2025
Comment on lines
-96
to
-97
| private static final LoadingCache<String, MaybeUri> uriCache = CacheStats.of( | ||
| SharedTaggedMetricRegistries.getSingleton(), "dialogue-uri") |
There was a problem hiding this comment.
I've dropped this instrumentation since we don't have a 1-1 replacement using the same timeseries names for guava caches. If this unblocks android stuff and we move forward, we can look into adding some guava cache utility functionality.
ellisjoe
force-pushed
the
jellis/guava-caches
branch
from
f093736 to
4cbd124
Compare
reduce coupling to caffeine in favor of guava We generally prefer caffeine caches, however they have compatibility issues with graalvm-native as well as android codebases. replace missed caches, drop some instrumentation
ellisjoe
force-pushed
the
jellis/guava-caches
branch
from
4cbd124 to
e103e7d
Compare
carterkozak
previously approved these changes
Jan 30, 2025
[skip ci]
policy-bot
bot
dismissed
carterkozak’s stale review
Invalidated by push of 5ce0515
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.
Before this PR
Caffeine caches run into issues when running on Android, minimally related to its usage of
java.lang.System$Loggerwhich isn't present.After this PR
Swap over to using Guava caches which should be fully compatible on Android devices.
==COMMIT_MSG==
==COMMIT_MSG==
Possible downsides?
Ideally this change would be done in a way where we pick Caffeine caches when running on standard JVMs and Guava caches when running on Android devices.