-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
Make MockLogAppender threadsafe #108206
Make MockLogAppender threadsafe #108206
Conversation
Adding and removing appenders in Log4j is not threadsafe. Yet some tests rely on capturing logging by adding an in memory appender, MockLogAppender. This commit makes the mock logging threadsafe by creating a new, singular appender for mock logging that delegates, in a threadsafe way, to the existing appenders created. Confusingly MockLogAppender is no longer really an appender, but I'm leaving clarifying that for a followup so as to limit the scope of this PR. closes elastic#106425
Pinging @elastic/es-core-infra (Team:Core/Infra) |
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.
Looks very nice, I have left a couple of questions
@@ -31,12 +33,35 @@ | |||
/** | |||
* Test appender that can be used to verify that certain events were logged correctly | |||
*/ | |||
public class MockLogAppender extends AbstractAppender { | |||
public class MockLogAppender { |
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.
Q: since this is no longer an appender, could this break things in serverless? (if tests there use something like Loggers.addAppender(mockAppender)
)
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.
It will "break" things, but I have the necessary changes ready.
mockAppenders.compute(logger, (k, v) -> { | ||
assert v != null; | ||
v.remove(this); | ||
return v.isEmpty() ? null : v; |
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.
TIL: compute remappingFunction returning null removes the mapping. Nice!
|
||
private static final Logger logger = LogManager.getLogger(MockLogAppender.class); |
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 don't understand why do we need this. What I'm missing?
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.
Leftover from debugging, will remove
@@ -259,6 +259,7 @@ public static void resetPortCounter() { | |||
// TODO: consolidate logging initialization for tests so it all occurs in logconfigurator | |||
LogConfigurator.loadLog4jPlugins(); | |||
LogConfigurator.configureESLogging(); | |||
MockLogAppender.init(); |
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.
This needs to be called every time we (re) configure log4j in tests, right?
So besides the special case above, ESTestCase (should) cover all, correct?
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.
Yep, any tests subclassing LuceneTestCase directly will need to call it if they use MockLogAppender.
private final List<WrappedLoggingExpectation> expectations; | ||
private final AtomicBoolean isAlive = new AtomicBoolean(true); |
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.
Does this need to be an AtomicBoolean? Couldnt it just be a volatile boolean
?
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.
Same in WrappedLoggingExpectation
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.
LGTM
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.
LGTM2 but I haven't looked at this very deeply so I'll leave the final approval to the other reviewers.
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.
LGTM 2! Nice change!
(btw, CI test failure looks related but should be fixable) |
Adding and removing appenders in Log4j is not threadsafe. Yet some tests rely on capturing logging by adding an in memory appender, MockLogAppender. This commit makes the mock logging threadsafe by creating a new, singular appender for mock logging that delegates, in a threadsafe way, to the existing appenders created. Confusingly MockLogAppender is no longer really an appender, but I'm leaving clarifying that for a followup so as to limit the scope of this PR. closes elastic#106425
Adding and removing appenders in Log4j is not threadsafe. Yet some tests rely on capturing logging by adding an in memory appender, MockLogAppender. This commit makes the mock logging threadsafe by creating a new, singular appender for mock logging that delegates, in a threadsafe way, to the existing appenders created. Confusingly MockLogAppender is no longer really an appender, but I'm leaving clarifying that for a followup so as to limit the scope of this PR.
closes #106425