-
Notifications
You must be signed in to change notification settings - Fork 150
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
JdbcHelper urlTofactory Map replacement #1114
Conversation
...t-bridge-datastore/src/main/java/com/newrelic/agent/bridge/datastore/ExpiringValueCache.java
Show resolved
Hide resolved
|
||
private final V wrappedValue; | ||
|
||
private final Instant timeCreated; |
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.
can these be longs?
it would save some allocation from the heap.
ExpiringValueLogicFunction expireLogicFunction) { | ||
wrappedMap = new ConcurrentHashMap<>(); | ||
|
||
TimerTask task = new TimerTask() { |
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.
Could we use a static ScheduledThreadPoolExecutor?
Or else we'd be creating and holding one Thread per cache.
@meiao Comments addressed:
|
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.
+1 lgtm
I only have one question that is just for my own understanding. Was there a specific reason to implement this custom cache vs using the caffeine dependency we have? Or are we doing something with the timestamp value that isn't available in caffeine?
It is not trivial to use Caffeine in this module due to the way it is loaded to the JVM. |
* WIP * Refactoring of cache class; more tests * Address PR comments: Instant --> long; static executor
Replaces the existing ConcurrentHashMap used for the
urlToFactory
andurlToDatabaseName
maps with a simple, custom cache that can expire items in the cache based on the last time an item was accessed.The ConcurrentHashMap implementations were causing issues when used with MySql replication, where URLs to target replica databases were comprised of unique, generated Strings, causing the maps to grow without bound, which could eventually cause an OOM exception.