You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "itholic (via GitHub)" <gi...@apache.org> on 2024/04/08 03:49:00 UTC
[PR] [SPARK-47590][SQL] Hive-thriftserver: Migrate logWarn with variables to structured logging framework [spark]
itholic opened a new pull request, #45923:
URL: https://github.com/apache/spark/pull/45923
### What changes were proposed in this pull request?
This PR proposes to migrate `logWarning` with variables of Hive module to Hive-thriftserver.
### Why are the changes needed?
To improve the existing logging system by migrating into structured logging.
### Does this PR introduce _any_ user-facing change?
No API changes, but the SQL catalyst logs will contain MDC(Mapped Diagnostic Context) from now.
### How was this patch tested?
Run Scala auto formatting and style check. Also the existing CI should pass.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47590][SQL] Hive-thriftserver: Migrate logWarn with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #45923:
URL: https://github.com/apache/spark/pull/45923#discussion_r1566429204
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/ui/HiveThriftServer2Listener.scala:
##########
@@ -160,8 +163,9 @@ private[thriftserver] class HiveThriftServer2Listener(
case Some(sessionData) =>
sessionData.totalExecution += 1
updateLiveStore(sessionData)
- case None => logWarning(s"onOperationStart called with unknown session id: ${e.sessionId}." +
- s"Regardless, the operation has been registered.")
+ case None => logWarning(
+ log"onOperationStart called with unknown session id: ${MDC(STATEMENT_ID, e.sessionId)}." +
Review Comment:
```suggestion
log"onOperationStart called with unknown session id: ${MDC(SESSION_ID, e.sessionId)}." +
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47590][SQL] Hive-thriftserver: Migrate logWarn with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #45923:
URL: https://github.com/apache/spark/pull/45923#discussion_r1566427959
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLCLIDriver.scala:
##########
@@ -232,14 +232,15 @@ private[hive] object SparkSQLCLIDriver extends Logging {
val historyFile = historyDirectory + File.separator + ".hivehistory"
reader.setHistory(new FileHistory(new File(historyFile)))
} else {
- logWarning("WARNING: Directory for Hive history file: " + historyDirectory +
- " does not exist. History will not be available during this session.")
+ logWarning(
+ log"Directory for Hive history file: ${MDC(HISTORY_DIR, historyDirectory)}" +
+ log" does not exist. History will not be available during this session.")
}
} catch {
case e: Exception =>
- logWarning("WARNING: Encountered an error while trying to initialize Hive's " +
- "history file. History will not be available during this session.")
- logWarning(e.getMessage)
+ logWarning("Encountered an error while trying to initialize Hive's " +
+ "history file. History will not be available during this session.", e)
+ logWarning(log"${MDC(ERROR, e.getMessage)}", e)
Review Comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47590][SQL] Hive-thriftserver: Migrate logWarn with variables to structured logging framework [spark]
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #45923:
URL: https://github.com/apache/spark/pull/45923#discussion_r1555367380
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLCLIDriver.scala:
##########
@@ -231,14 +232,15 @@ private[hive] object SparkSQLCLIDriver extends Logging {
val historyFile = historyDirectory + File.separator + ".hivehistory"
reader.setHistory(new FileHistory(new File(historyFile)))
} else {
- logWarning("WARNING: Directory for Hive history file: " + historyDirectory +
- " does not exist. History will not be available during this session.")
+ logWarning(
+ log"WARNING: Directory for Hive history file: ${MDC(HISTORY_DIR, historyDirectory)}" +
+ log" does not exist. History will not be available during this session.")
Review Comment:
Sure. I was just trying to keep the existing error message as it is, but it would be better to have only keep one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47590][SQL] Hive-thriftserver: Migrate logWarn with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #45923:
URL: https://github.com/apache/spark/pull/45923#discussion_r1560444898
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/ui/HiveThriftServer2Listener.scala:
##########
@@ -218,7 +232,9 @@ private[thriftserver] class HiveThriftServer2Listener(
executionData.state = ExecutionState.CLOSED
updateStoreWithTriggerEnabled(executionData)
executionList.remove(e.id)
- case None => logWarning(s"onOperationClosed called with unknown operation id: ${e.id}")
+ case None => logWarning(
+ log"onOperationClosed called with unknown operation id: ${MDC(OP_ID, e.id)}"
Review Comment:
From the call side, the id is statement_id
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47590][SQL] Hive-thriftserver: Migrate logWarn with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #45923:
URL: https://github.com/apache/spark/pull/45923#discussion_r1560115778
##########
common/utils/src/main/scala/org/apache/spark/internal/LogKey.scala:
##########
@@ -100,6 +101,7 @@ object LogKey extends Enumeration {
val OFFSETS = Value
val OLD_BLOCK_MANAGER_ID = Value
val OLD_VALUE = Value
+ val OPERATION_ID = Value
Review Comment:
this duplicateds with OP_ID
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47590][SQL] Hive-thriftserver: Migrate logWarn with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on PR #45923:
URL: https://github.com/apache/spark/pull/45923#issuecomment-2059908947
Thanks, merging to master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47590][SQL] Hive-thriftserver: Migrate logWarn with variables to structured logging framework [spark]
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #45923:
URL: https://github.com/apache/spark/pull/45923#discussion_r1560460548
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/ui/HiveThriftServer2Listener.scala:
##########
@@ -218,7 +232,9 @@ private[thriftserver] class HiveThriftServer2Listener(
executionData.state = ExecutionState.CLOSED
updateStoreWithTriggerEnabled(executionData)
executionList.remove(e.id)
- case None => logWarning(s"onOperationClosed called with unknown operation id: ${e.id}")
+ case None => logWarning(
+ log"onOperationClosed called with unknown operation id: ${MDC(OP_ID, e.id)}"
Review Comment:
That means that we should change all `OP_ID` into `STATEMENT_ID` from this file, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47590][SQL] Hive-thriftserver: Migrate logWarn with variables to structured logging framework [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #45923:
URL: https://github.com/apache/spark/pull/45923#discussion_r1555253322
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLCLIDriver.scala:
##########
@@ -231,14 +232,15 @@ private[hive] object SparkSQLCLIDriver extends Logging {
val historyFile = historyDirectory + File.separator + ".hivehistory"
reader.setHistory(new FileHistory(new File(historyFile)))
} else {
- logWarning("WARNING: Directory for Hive history file: " + historyDirectory +
- " does not exist. History will not be available during this session.")
+ logWarning(
+ log"WARNING: Directory for Hive history file: ${MDC(HISTORY_DIR, historyDirectory)}" +
+ log" does not exist. History will not be available during this session.")
Review Comment:
There are several `whitespaces` before `History`, should we only keep one?
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLCLIDriver.scala:
##########
@@ -231,14 +232,15 @@ private[hive] object SparkSQLCLIDriver extends Logging {
val historyFile = historyDirectory + File.separator + ".hivehistory"
reader.setHistory(new FileHistory(new File(historyFile)))
} else {
- logWarning("WARNING: Directory for Hive history file: " + historyDirectory +
- " does not exist. History will not be available during this session.")
+ logWarning(
+ log"WARNING: Directory for Hive history file: ${MDC(HISTORY_DIR, historyDirectory)}" +
+ log" does not exist. History will not be available during this session.")
}
} catch {
case e: Exception =>
logWarning("WARNING: Encountered an error while trying to initialize Hive's " +
Review Comment:
dito
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47590][SQL] Hive-thriftserver: Migrate logWarn with variables to structured logging framework [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #45923:
URL: https://github.com/apache/spark/pull/45923#discussion_r1555253496
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLCLIDriver.scala:
##########
@@ -231,14 +232,15 @@ private[hive] object SparkSQLCLIDriver extends Logging {
val historyFile = historyDirectory + File.separator + ".hivehistory"
reader.setHistory(new FileHistory(new File(historyFile)))
} else {
- logWarning("WARNING: Directory for Hive history file: " + historyDirectory +
- " does not exist. History will not be available during this session.")
+ logWarning(
+ log"WARNING: Directory for Hive history file: ${MDC(HISTORY_DIR, historyDirectory)}" +
+ log" does not exist. History will not be available during this session.")
}
} catch {
case e: Exception =>
logWarning("WARNING: Encountered an error while trying to initialize Hive's " +
- "history file. History will not be available during this session.")
- logWarning(e.getMessage)
+ "history file. History will not be available during this session.", e)
Review Comment:
dito
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLCLIDriver.scala:
##########
@@ -249,7 +251,8 @@ private[hive] object SparkSQLCLIDriver extends Logging {
h.flush()
} catch {
case e: IOException =>
- logWarning("WARNING: Failed to write command history file: " + e.getMessage)
+ logWarning(
+ log"WARNING: Failed to write command history file: ${MDC(ERROR, e.getMessage)}", e)
Review Comment:
dito
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47590][SQL] Hive-thriftserver: Migrate logWarn with variables to structured logging framework [spark]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #45923:
URL: https://github.com/apache/spark/pull/45923#issuecomment-2041889112
LGTM. Actually, there are lot of Java code copied from Hive in this module, I'm not sure if we want to do some refactoring to adapt the structured logging framework.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47590][SQL] Hive-thriftserver: Migrate logWarn with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #45923:
URL: https://github.com/apache/spark/pull/45923#discussion_r1563384223
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/ui/HiveThriftServer2Listener.scala:
##########
@@ -218,7 +232,9 @@ private[thriftserver] class HiveThriftServer2Listener(
executionData.state = ExecutionState.CLOSED
updateStoreWithTriggerEnabled(executionData)
executionList.remove(e.id)
- case None => logWarning(s"onOperationClosed called with unknown operation id: ${e.id}")
+ case None => logWarning(
+ log"onOperationClosed called with unknown operation id: ${MDC(OP_ID, e.id)}"
Review Comment:
Probably, please double check the callsites to make sure we are using the right variable name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47590][SQL] Hive-thriftserver: Migrate logWarn with variables to structured logging framework [spark]
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on PR #45923:
URL: https://github.com/apache/spark/pull/45923#issuecomment-2041811506
cc @gengliangwang as you're working on logError for Hive-thriftserver IIRC.
Also cc @panbingkun @pan3793
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47590][SQL] Hive-thriftserver: Migrate logWarn with variables to structured logging framework [spark]
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #45923:
URL: https://github.com/apache/spark/pull/45923#discussion_r1555389485
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/ui/HiveThriftServer2Listener.scala:
##########
@@ -218,7 +232,9 @@ private[thriftserver] class HiveThriftServer2Listener(
executionData.state = ExecutionState.CLOSED
updateStoreWithTriggerEnabled(executionData)
executionList.remove(e.id)
- case None => logWarning(s"onOperationClosed called with unknown operation id: ${e.id}")
+ case None => logWarning(
+ log"onOperationClosed called with unknown operation id: ${MDC(OPERATION_ID, e.id)}"
Review Comment:
Maybe can we keep the current behavior as it is?
I'm a bit afraid that it will even become more complicated because most of them are slightly different except for the “called with unknown” part.
For example, we might need at least 3 parameters for the common method would look like:
```scala
private def logWarningWithUnknownId(eventType: String, id: String, additionalMessage: String = ""): Unit = {
val idType = eventType match {
case "operation" => OPERATION_ID
case "session" => SESSION_ID
}
logWarning(log"$eventType called with unknown $idType: ${MDC(idType, id)}." + additionalMessage)
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47590][SQL] Hive-thriftserver: Migrate logWarn with variables to structured logging framework [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #45923:
URL: https://github.com/apache/spark/pull/45923#discussion_r1555443549
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/ui/HiveThriftServer2Listener.scala:
##########
@@ -218,7 +232,9 @@ private[thriftserver] class HiveThriftServer2Listener(
executionData.state = ExecutionState.CLOSED
updateStoreWithTriggerEnabled(executionData)
executionList.remove(e.id)
- case None => logWarning(s"onOperationClosed called with unknown operation id: ${e.id}")
+ case None => logWarning(
+ log"onOperationClosed called with unknown operation id: ${MDC(OPERATION_ID, e.id)}"
Review Comment:
Okay
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47590][SQL] Hive-thriftserver: Migrate logWarn with variables to structured logging framework [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #45923:
URL: https://github.com/apache/spark/pull/45923#discussion_r1555252729
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLCLIDriver.scala:
##########
@@ -231,14 +232,15 @@ private[hive] object SparkSQLCLIDriver extends Logging {
val historyFile = historyDirectory + File.separator + ".hivehistory"
reader.setHistory(new FileHistory(new File(historyFile)))
} else {
- logWarning("WARNING: Directory for Hive history file: " + historyDirectory +
- " does not exist. History will not be available during this session.")
+ logWarning(
+ log"WARNING: Directory for Hive history file: ${MDC(HISTORY_DIR, historyDirectory)}" +
Review Comment:
Nit, I feel like we can remove this text `WARNING`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47590][SQL] Hive-thriftserver: Migrate logWarn with variables to structured logging framework [spark]
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #45923:
URL: https://github.com/apache/spark/pull/45923#discussion_r1560187499
##########
common/utils/src/main/scala/org/apache/spark/internal/LogKey.scala:
##########
@@ -100,6 +101,7 @@ object LogKey extends Enumeration {
val OFFSETS = Value
val OLD_BLOCK_MANAGER_ID = Value
val OLD_VALUE = Value
+ val OPERATION_ID = Value
Review Comment:
Updated. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47590][SQL] Hive-thriftserver: Migrate logWarn with variables to structured logging framework [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #45923:
URL: https://github.com/apache/spark/pull/45923#discussion_r1555255130
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/ui/HiveThriftServer2Listener.scala:
##########
@@ -218,7 +232,9 @@ private[thriftserver] class HiveThriftServer2Listener(
executionData.state = ExecutionState.CLOSED
updateStoreWithTriggerEnabled(executionData)
executionList.remove(e.id)
- case None => logWarning(s"onOperationClosed called with unknown operation id: ${e.id}")
+ case None => logWarning(
+ log"onOperationClosed called with unknown operation id: ${MDC(OPERATION_ID, e.id)}"
Review Comment:
There are too many similar `... called with unknown operation id: ${MDC(OPERATION_ID, e.id)}` in here, perhaps we can extract a common method?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47590][SQL] Hive-thriftserver: Migrate logWarn with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang closed pull request #45923: [SPARK-47590][SQL] Hive-thriftserver: Migrate logWarn with variables to structured logging framework
URL: https://github.com/apache/spark/pull/45923
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47590][SQL] Hive-thriftserver: Migrate logWarn with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #45923:
URL: https://github.com/apache/spark/pull/45923#discussion_r1566428306
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLCLIDriver.scala:
##########
@@ -250,7 +251,8 @@ private[hive] object SparkSQLCLIDriver extends Logging {
h.flush()
} catch {
case e: IOException =>
- logWarning("WARNING: Failed to write command history file: " + e.getMessage)
+ logWarning(
+ log"Failed to write command history file: ${MDC(ERROR, e.getMessage)}", e)
Review Comment:
```suggestion
log"Failed to write command history file: ${MDC(ERROR, e.getMessage)}")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org