CASSANDRA-21474: NoSpamLogger uses unbounded cache that could lead to memory exhaustion - #4993
CASSANDRA-21474: NoSpamLogger uses unbounded cache that could lead to memory exhaustion#4993viktoriiakotovets wants to merge 2 commits into
Conversation
…port from CNDB-17505) ### What is the issue NoSpamLogger uses unbounded cache that could lead to memory exhaustion ### What does this PR fix and why was it fixed This PR replaces the previous `NonBlockingHashMap` based caching implementation in `NoSpamLogger` with Caffeine cache to prevent unbounded memory growth and improve cache management
| .expireAfter(new Expiry<String, NoSpamLogStatement>() | ||
| { | ||
| @Override | ||
| public long expireAfterCreate(String key, NoSpamLogStatement value, long currentTime) | ||
| { | ||
| return value.expiry(); | ||
| } | ||
|
|
||
| @Override | ||
| public long expireAfterUpdate(String key, NoSpamLogStatement value, | ||
| long currentTime, long currentDuration) | ||
| { | ||
| return value.expiry(); | ||
| } | ||
|
|
||
| @Override | ||
| public long expireAfterRead(String key, NoSpamLogStatement value, | ||
| long currentTime, long currentDuration) | ||
| { | ||
| return currentDuration; | ||
| } | ||
| }) |
There was a problem hiding this comment.
fwiw, you might prefer
expireAfter(Expiry.writing((String key, NoSpamLogStatement value) -> Duration.ofNanos(value.expiry()))There was a problem hiding this comment.
sure, but that might need upgrade to 3.2.2 first
| } | ||
| }) | ||
| .ticker(TICKER) | ||
| .executor(ForkJoinPool.commonPool()) |
There was a problem hiding this comment.
@viktoriiakotovets I am not completely sure about this executor here. If you look what executors we use for Caffeine caches we never used this one. It would be appropriate if you did some basic research for the justification why we should use this executor specifically or change it to something more fitting.
There was a problem hiding this comment.
right, we don't want to use commonPool() (which can be limited in threads and easily starved).
@smiklosovic , which existing executor would you recommend ? or should we create a new one ?
There was a problem hiding this comment.
If we want a dedicated executor for this then I would go with org.apache.cassandra.concurrent.ExecutorFactory.Global.executorFactory().sequential("NoSpamLogger") otherwise I would go for ScheduledExecutors.optionalTasks or ImmediateExecutor.INSTANCE.
I lean towards a dedicated one because I do not like the fact that optionalTasks / INSTANCE would be used for executing stuff for a logger while that executor is used quite intensively for Cassandra itself and I do not want to mix it.
I would also most probably use executorFactory().withJmxInternal().sequential("NoSpamLogger"), withJMXInternal should give us the way how to query metrics of this executor via JMX so we have a visibility into what it is doing which is not a must but still a nice to have.
That being said, we would need to walk an extra mile here to be sure that we shutdown the executor upon shutdown of a node. AFAIK Caffeine is not shutting down the executor we hand it so we would need to be sure that we shut it down when not used anymore, likely in something like StorageService.drain() or similar.
btw isnt ForkJoinPool.commonPool() the default when we dont set it? So us setting it here is actually redundant. https://github.com/ben-manes/caffeine/blob/v3.1.8/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Caffeine.java#L337
| public void testNoSpamLogStatementCacheBounded() | ||
| { | ||
| int maxStatementsPerLogger = 10; | ||
| System.setProperty("cassandra.nospam_logger.max_statements_per_logger", String.valueOf(maxStatementsPerLogger)); |
There was a problem hiding this comment.
could you rewrite this to more idiomatic Cassandra style of:
try (WithProperties properties = new WithProperties().set(CassandraRelevantProperties.NOSPAM_LOGGER_MAX_STATEMENTS_PER_LOGGER,
String.valueOf(maxStatementsPerLogger)))
{
// test here, you do not need to reset manually,
// WithProperties will reset in AutoCloseable
}
finally
{
NoSpamLogger.clearWrappedLoggersForTest();
}
5 participants
https://issues.apache.org/jira/browse/CASSANDRA-21474): NoSpamLogger uses unbounded cache that could lead to memory exhaustion
What is the issue
NoSpamLogger uses unbounded cache that could lead to memory exhaustionWhat does this PR fix and why was it fixed
This PR replaces the previous NonBlockingHashMap based caching implementation in NoSpamLogger with Caffeine cache to prevent unbounded memory growth and improve cache management