SlowLoggers using single logger #56708
Conversation
Slow loggers should use single shared logger as otherwise when index is deleted the log4j logger will remain reachable (log4j is caching) and will create a memory leak.
pgomulka added :Core/Infra/Logging 
| Pinging @elastic/es-core-infra (:Core/Infra/Logging) |
| @elasticmachine run elasticsearch-ci/1 |
server/src/main/java/org/elasticsearch/index/IndexingSlowLog.java Outdated Show resolved Hide resolved
server/src/test/java/org/elasticsearch/common/logging/MockAppender.java Outdated Show resolved Hide resolved
server/src/test/java/org/elasticsearch/common/logging/MockAppender.java Outdated Show resolved Hide resolved
server/src/test/java/org/elasticsearch/index/SlowLogLevelTest.java Outdated Show resolved Hide resolved
jasontedor left a comment
jasontedor left a comment There was a problem hiding this comment.
I left one comment relating the representation of specificity. Also, I think we should have a test that the memory leak is no longer present? That is creating N indices doesn't result in N slow loggers?
Otherwise, this is looking great.
| private long indexInfoThreshold; | ||
| private long indexDebugThreshold; | ||
| private long indexTraceThreshold; | ||
| /** |
There was a problem hiding this comment.
A nit here, sorry: I know this is carried over from the code that you moved, but this is labeled as a Javadoc /** on a private field. It should be a regular comment /*. Also no reason to not make this comment use the full 140 columns that are available. 😇
| WARN(3), //most specific - little logging | ||
| INFO(2), | ||
| DEBUG(1), | ||
| TRACE(0); //least specific - lots of logging |
There was a problem hiding this comment.
Nit: space between the // and the start of the comment
| IndexingSlowLog log2 = new IndexingSlowLog(index2Settings); | ||
| context = (LoggerContext) LogManager.getContext(false); | ||
| | ||
| int numberOfLoggersAfter = context.getLoggers().size(); |
There was a problem hiding this comment.
Maybe we can do this a random number of times, in a loop, say up to 64 or something like that, just to really exercise that no new ones are created?
jasontedor left a comment
jasontedor left a comment There was a problem hiding this comment.
LGTM. I left a few comments. No need for another review round though.
Slow loggers should use single shared logger as otherwise when index is deleted the log4j logger will remain reachable (log4j is caching) and will create a memory leak. closes elastic#56171
Slow loggers should use single shared logger as otherwise when index is deleted the log4j logger will remain reachable (log4j is caching) and will create a memory leak. closes elastic#56171
Slow loggers should use single shared logger as otherwise when index is deleted the log4j logger will remain reachable (log4j is caching) and will create a memory leak. closes elastic#56171
Slow loggers should use single shared logger as otherwise when index is deleted the log4j logger will remain reachable (log4j is caching) and will create a memory leak. closes elastic#56171
| I'm removing the Backport Pending label since this appears to merged into all the labeled branches. |
5 participants
Slow loggers should use single shared logger as otherwise when index is
deleted the log4j logger will remain reachable (log4j is caching) and
will create a memory leak.
closes #56171