-
Notifications
You must be signed in to change notification settings - Fork 28.2k
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
[SPARK-48744][Core] Log entry should be constructed only once #47135
Conversation
cc @panbingkun |
@@ -228,6 +228,37 @@ trait LoggingSuiteBase | |||
verifyMsgWithConcat(level, logOutput) | |||
} | |||
} | |||
|
|||
test("LogEntry should construct MessageWithContext only once") { |
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.
Perhaps we should put the above testing logic into MDCSuite
or a new UT (eg: MessageWithContextSuite
), as it seems unrelated to the classification of structured
and pattern
logs, so it will be executed twice in GA?
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.
A test takes 200ms, running twice is ok. Also, we need to ensure the log entry is constructed only once when structured logging is disabled, too.
This is really a great improvement! |
Merging to master |
### What changes were proposed in this pull request? In the current implementation: ``` class LogEntry(messageWithContext: => MessageWithContext) { def message: String = messageWithContext.message def context: java.util.HashMap[String, String] = messageWithContext.context } def logInfo(entry: LogEntry): Unit = { if (log.isInfoEnabled) { withLogContext(entry.context) { log.info(entry.message) } } } ``` The field `messageWithContext` is constructed twice, one from `entry.context` and another one from `entry.message`. This PR is to improve this and ensure a log entry is constructed only once. ### Why are the changes needed? Improve the performance of logging ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? New UT ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#47135 from gengliangwang/addCache. Authored-by: Gengliang Wang <[email protected]> Signed-off-by: Gengliang Wang <[email protected]>
What changes were proposed in this pull request?
In the current implementation:
The field
messageWithContext
is constructed twice, one fromentry.context
and another one fromentry.message
.This PR is to improve this and ensure a log entry is constructed only once.
Why are the changes needed?
Improve the performance of logging
Does this PR introduce any user-facing change?
No
How was this patch tested?
New UT
Was this patch authored or co-authored using generative AI tooling?
No