-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-26947 Implement a special TestAppender to limit the size of tes… #4340
Conversation
It is not easy to write a UT for the log4j2 appender so I just test it manually. Change the 'appender.console.maxSize' property in log4j2.properties to a very small value, for example, 1K, you will soon see this output when running UT in IDE
So I think this approach works. And I set default value to 1G, which should be enough for most cases... @busbey @ndimiduk FYI. I think this could solve the recent space problem of our ci system. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
public void append(LogEvent event) { | ||
if (stop.get()) { | ||
return; | ||
} | ||
// for accounting, here we always convert the event to byte array first | ||
// this will effect performance a bit but it is OK since this is for UT only | ||
byte[] bytes = getLayout().toByteArray(event); | ||
if (bytes == null || bytes.length == 0) { | ||
return; | ||
} | ||
long sizeAfterAppend = size.addAndGet(bytes.length); | ||
if (sizeAfterAppend >= maxSize) { | ||
// stop logging if the log size exceeded the limit | ||
if (stop.compareAndSet(false, true)) { | ||
LOGGER.error("Log size exceeded the limit {}, will stop logging to prevent eating" | ||
+ " too much disk space", maxSize); | ||
} | ||
return; | ||
} | ||
super.append(event); | ||
} | ||
} |
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 we abort the test in this case rather than having the test continue without logs?
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.
Throw an Error out maybe? In general I'm not sure if this could work, because it could be a thread other than the main thread which enters here and exceeds the limit, fail this thread can not abort the test?
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've done this in another logging framework but not log4j2. This is appending events to a single log line, building it up, is that correct? I think it's fine if we truncate log lines at a certain length, no need to error or whatever, just terminate the message. I'm a little surprised that this not available to use as a configuration option.
In fact, the pattern layout does support it. https://stackoverflow.com/questions/27502536/limit-max-message-size-in-log4j2-pattern
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.
Or maybe it's one log line per Event object. I see.
I'm fine with this. I think it's better to let the test finish than to kill the build when the log limit is exceeded. In my experience, it's relatively rare that logs from CI are used by anyone. This strategy for logging once max log volume for a test is exceeded is fine by me.
What happens when the appender is for some class that's used over and over in test? There's no way to reset this counter to be a per-test-class accumulator, right?
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.
We do not use reuseForks so this is not a problem. Every test will have its own process.
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.
We do not use reuseForks so this is not a problem. Every test will have its own process.
In that case, it would be better to add an assertion that this invariant is held. With that, I'm +1.
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.
If asserting on the invariant is impossible, I'm okay with adding a comment here so that at least a future change to test execution will have a breadcrumb to follow.
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 do not know how to add the assertion in code to test a pom config :(
But anyway, we could add a comment about this, both in the appender code and also in pom.
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've tried to use RunListener to reset the counter when starting a new test but failed... The behavior is really confusing, I mean when will the methods in the RunListener be called, and when will the surefire plugin rotate the output tests when reuseForks is true...
So I just add some comments to pom.xml to say that why we can not set reuseForks to true.
PTAL. @ndimiduk
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Ping @ndimiduk |
…t output