Skip to content

[SPARK-48490][CORE][FOLLOWUP] Fix Unescape about MessageWithContext #47029

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
panbingkun wants to merge 1 commit into from
Closed

[SPARK-48490][CORE][FOLLOWUP] Fix Unescape about MessageWithContext #47029

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 10 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 @@ -19,7 +19,6 @@ package org.apache.spark.internal

import scala.jdk.CollectionConverters._

import org.apache.commons.text.StringEscapeUtils
import org.apache.logging.log4j.{CloseableThreadContext, Level, LogManager}
import org.apache.logging.log4j.core.{Filter, LifeCycle, LogEvent, Logger => Log4jLogger, LoggerContext}
import org.apache.logging.log4j.core.appender.ConsoleAppender
Expand Down Expand Up @@ -100,7 +99,7 @@ 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 = StringEscapeUtils.unescapeJava(messageWithContext.message)
Copy link
Member

@HyukjinKwon HyukjinKwon Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we maybe simply try-catch, and return the original string if it fails?

Copy link
Member

@HyukjinKwon HyukjinKwon Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows users are arguably less in any event.

Copy link
Member

@HyukjinKwon HyukjinKwon Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and logging shouldn't fail the application anyway.

Copy link
Contributor Author

@panbingkun panbingkun Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm..., this may have successfully unescaped, but the result does not meet expectations, such as:

println(StringEscapeUtils.unescapeJava("C:\\Users\\runneradmin\\AppData\\Local\\remp\\spark-9f4b7e74-ea9e-46fc-bde6-1449cc9f6e8e"))

We may expect it to output as:

C:\Users\runneradmin\AppData\Local\remp\spark-9f4b7e74-ea9e-46fc-bde6-1449cc9f6e8e

Actual, its output is:

empspark-9f4b7e74-ea9e-46fc-bde6-1449cc9f6e8e

So it's better not to execute the unescape operation and let the user manually use the method I wrote in UT to achieve its expected behavior, although it does look a bit ugly.

Copy link
Contributor Author

@panbingkun panbingkun Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and logging shouldn't fail the application anyway.

Yes, we can use try ... catch temporarily fixes it, but it has some of my flaws mentioned above.
Or should we do this (use try ... catch) first and let build_sparkr_window pass?

Copy link
Member

@HyukjinKwon HyukjinKwon Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm.

Copy link
Member

@HyukjinKwon HyukjinKwon Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with this approach but I think we need @gengliangwang 's look at least.

Copy link
Contributor Author

@panbingkun panbingkun Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, let's wait for @gengliangwang's review.
Thanks!

def message: String = messageWithContext.message

def context: java.util.HashMap[String, String] = messageWithContext.context
}
Expand Down Expand Up @@ -161,8 +160,17 @@ trait Logging {

MessageWithContext(sb.toString(), context)
}

def log(c: Char): MessageWithContext = {
Copy link
Contributor Author

@panbingkun panbingkun Jun 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have provided a new method for the character

MessageWithContext(s"$c", new java.util.HashMap[String, String]())
}
}

private final val LF_CHAR: Char = '\n'
private final val TAB_CHAR: Char = '\t'
final val LF = MessageWithContext(s"$LF_CHAR", new java.util.HashMap[String, String]())
Copy link
Member

@gengliangwang gengliangwang Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait..are you suggesting that we should use this constant in log messages? If so, I would prefer try...catch as @HyukjinKwon suggested.

Copy link
Contributor Author

@panbingkun panbingkun Jun 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • This is just to make it easier for developers to use commonly used escape characters, such as:
    line break - LF_CHAR, tab - TAB_CHAR
    Of course, we can also use it this way:
    image

  • Additional explanation, if we write it as follows, it will exhibit unexpected behavior:
    image
    Because it is written in this way, in our framework it will consider "\n" as a string rather than a character
    image
    that is to say:

    s"\n" != log"\n".message

final val TAB = MessageWithContext(s"$TAB_CHAR", new java.util.HashMap[String, String]())

protected def withLogContext(context: java.util.HashMap[String, String])(body: => Unit): Unit = {
val threadContext = CloseableThreadContext.putAll(context)
try {
Expand Down
2 changes: 1 addition & 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 @@ -56,7 +56,7 @@ trait LoggingSuiteBase
def basicMsgWithEscapeChar: String = "This is a log message\nThis is a new line \t other msg"

def basicMsgWithEscapeCharMDC: LogEntry =
log"This is a log message\nThis is a new line \t other msg"
log"This is a log message" + LF + log"This is a new line " + TAB + log" other msg"
Copy link
Contributor Author

@panbingkun panbingkun Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although the above writing may seem a bit ugly, so far, the only method I can find that meets the requirements and is relatively acceptable


def msgWithMDC: LogEntry = log"Lost executor ${MDC(LogKeys.EXECUTOR_ID, "1")}."

Expand Down