Skip to content

[SPARK-48744][Core] Log entry should be constructed only once #47135

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.

Sign up for GitHub

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

Closed
gengliangwang wants to merge 2 commits into from
Closed

[SPARK-48744][Core] Log entry should be constructed only once #47135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
99c34ac
LogEntry should construct MessageWithContext only once
gengliangwang Jun 27, 2024
1364de1
add blank line
gengliangwang Jun 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions common/utils/src/main/scala/org/apache/spark/internal/Logging.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,11 @@ case class MessageWithContext(message: String, context: java.util.HashMap[String
* Companion class for lazy evaluation of the MessageWithContext instance.
*/
class LogEntry(messageWithContext: => MessageWithContext) {
def message: String = messageWithContext.message
private lazy val cachedMessageWithContext: MessageWithContext = messageWithContext

def context: java.util.HashMap[String, String] = messageWithContext.context
def message: String = cachedMessageWithContext.message

def context: java.util.HashMap[String, String] = cachedMessageWithContext.context
}

/**
Expand Down
33 changes: 32 additions & 1 deletion common/utils/src/test/scala/org/apache/spark/util/StructuredLoggingSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import com.fasterxml.jackson.module.scala.DefaultScalaModule
import org.apache.logging.log4j.Level
import org.scalatest.funsuite.AnyFunSuite // scalastyle:ignore funsuite

import org.apache.spark.internal.{LogEntry, Logging, LogKey, LogKeys, MDC}
import org.apache.spark.internal.{LogEntry, Logging, LogKey, LogKeys, MDC, MessageWithContext}

trait LoggingSuiteBase
extends AnyFunSuite // scalastyle:ignore funsuite
Expand Down Expand Up @@ -228,6 +228,37 @@ trait LoggingSuiteBase
verifyMsgWithConcat(level, logOutput)
}
}

test("LogEntry should construct MessageWithContext only once") {
Copy link
Contributor

@panbingkun panbingkun Jun 28, 2024

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?

Copy link
Member Author

@gengliangwang gengliangwang Jun 28, 2024

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.

var constructionCount = 0

def constructMessageWithContext(): MessageWithContext = {
constructionCount += 1
log"Lost executor ${MDC(LogKeys.EXECUTOR_ID, "1")}."
}
logInfo(constructMessageWithContext())
assert(constructionCount === 1)
}

test("LogEntry should construct MessageWithContext only once II") {
var constructionCount = 0
var constructionCount2 = 0

def executorId(): String = {
constructionCount += 1
"1"
}

def workerId(): String = {
constructionCount2 += 1
"2"
}

logInfo(log"Lost executor ${MDC(LogKeys.EXECUTOR_ID, executorId())}." +
log"worker id ${MDC(LogKeys.WORKER_ID, workerId())}")
assert(constructionCount === 1)
assert(constructionCount2 === 1)
}
}

class StructuredLoggingSuite extends LoggingSuiteBase {
Expand Down