Re: [PR] [SPARK-47594] Connector module: Migrate logInfo with variables to structured logging framework [spark]
panbingkun commented on code in PR #46022:
URL: https://github.com/apache/spark/pull/46022#discussion_r1590471939
##
connector/profiler/src/main/scala/org/apache/spark/executor/profiler/ExecutorProfilerPlugin.scala:
##
@@ -23,7 +23,8 @@ import scala.util.Random
import org.apache.spark.SparkConf
import org.apache.spark.api.plugin.{DriverPlugin, ExecutorPlugin,
PluginContext, SparkPlugin}
-import org.apache.spark.internal.Logging
+import org.apache.spark.internal.LogKey.EXECUTOR_ID
+import org.apache.spark.internal.{Logging, MDC}
Review Comment:
Thank you very much for fix it.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47594] Connector module: Migrate logInfo with variables to structured logging framework [spark]
dongjoon-hyun commented on code in PR #46022:
URL: https://github.com/apache/spark/pull/46022#discussion_r1589845846
##
connector/profiler/src/main/scala/org/apache/spark/executor/profiler/ExecutorJVMProfiler.scala:
##
@@ -25,7 +25,8 @@ import org.apache.hadoop.fs.{FileSystem, FSDataOutputStream,
Path}
import org.apache.spark.SparkConf
import org.apache.spark.deploy.SparkHadoopUtil
-import org.apache.spark.internal.Logging
+import org.apache.spark.internal.LogKey.PATH
+import org.apache.spark.internal.{Logging, MDC}
Review Comment:
Unfortunately, this import ordering issue was missed because
`dev/scalastyle` didn't include this module.
Here is the fix for `dev/scalastyle` and this.
- https://github.com/apache/spark/pull/46376
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47594] Connector module: Migrate logInfo with variables to structured logging framework [spark]
dongjoon-hyun commented on code in PR #46022:
URL: https://github.com/apache/spark/pull/46022#discussion_r1589845864
##
connector/profiler/src/main/scala/org/apache/spark/executor/profiler/ExecutorProfilerPlugin.scala:
##
@@ -23,7 +23,8 @@ import scala.util.Random
import org.apache.spark.SparkConf
import org.apache.spark.api.plugin.{DriverPlugin, ExecutorPlugin,
PluginContext, SparkPlugin}
-import org.apache.spark.internal.Logging
+import org.apache.spark.internal.LogKey.EXECUTOR_ID
+import org.apache.spark.internal.{Logging, MDC}
Review Comment:
ditto. Import ordering issue.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47594] Connector module: Migrate logInfo with variables to structured logging framework [spark]
gengliangwang closed pull request #46022: [SPARK-47594] Connector module: Migrate logInfo with variables to structured logging framework URL: https://github.com/apache/spark/pull/46022 -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47594] Connector module: Migrate logInfo with variables to structured logging framework [spark]
gengliangwang commented on PR #46022: URL: https://github.com/apache/spark/pull/46022#issuecomment-2059867235 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47594] Connector module: Migrate logInfo with variables to structured logging framework [spark]
panbingkun commented on code in PR #46022:
URL: https://github.com/apache/spark/pull/46022#discussion_r150002
##
connector/kafka-0-10/src/main/scala/org/apache/spark/streaming/kafka010/DirectKafkaInputDStream.scala:
##
@@ -325,7 +327,8 @@ private[spark] class DirectKafkaInputDStream[K, V](
override def restore(): Unit = {
batchForTime.toSeq.sortBy(_._1)(Time.ordering).foreach { case (t, b) =>
- logInfo(s"Restoring KafkaRDD for time $t ${b.mkString("[", ", ",
"]")}")
+ logInfo(log"Restoring KafkaRDD for time ${MDC(TIME, t)} " +
Review Comment:
Yeah, Here `t` is an instance of the `Time`, and `Time` defaults to
outputting a time unit of `ms`, as follows:
https://github.com/apache/spark/blob/e815012f26ab305030b170eb2f0aa28d2de843b6/streaming/src/main/scala/org/apache/spark/streaming/Time.scala#L87
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47594] Connector module: Migrate logInfo with variables to structured logging framework [spark]
panbingkun commented on PR #46022: URL: https://github.com/apache/spark/pull/46022#issuecomment-2058093392 > @panbingkun Thanks for the works. LGTM except for some minor comments. Updated. Thank you for your review! -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47594] Connector module: Migrate logInfo with variables to structured logging framework [spark]
gengliangwang commented on PR #46022: URL: https://github.com/apache/spark/pull/46022#issuecomment-2057848786 @panbingkun Thanks for the works. LGTM except for some minor comments. -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47594] Connector module: Migrate logInfo with variables to structured logging framework [spark]
gengliangwang commented on code in PR #46022:
URL: https://github.com/apache/spark/pull/46022#discussion_r1566462197
##
connector/kafka-0-10/src/main/scala/org/apache/spark/streaming/kafka010/DirectKafkaInputDStream.scala:
##
@@ -325,7 +327,8 @@ private[spark] class DirectKafkaInputDStream[K, V](
override def restore(): Unit = {
batchForTime.toSeq.sortBy(_._1)(Time.ordering).foreach { case (t, b) =>
- logInfo(s"Restoring KafkaRDD for time $t ${b.mkString("[", ", ",
"]")}")
+ logInfo(log"Restoring KafkaRDD for time ${MDC(TIME, t)} " +
Review Comment:
QQ: is the time here using ms?
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47594] Connector module: Migrate logInfo with variables to structured logging framework [spark]
gengliangwang commented on code in PR #46022:
URL: https://github.com/apache/spark/pull/46022#discussion_r1566459384
##
connector/kafka-0-10-sql/src/main/scala/org/apache/spark/sql/kafka010/consumer/KafkaDataConsumer.scala:
##
@@ -391,10 +392,12 @@ private[kafka010] class KafkaDataConsumer(
.getOrElse("")
val walTime = System.nanoTime() - startTimestampNano
-logInfo(
- s"From Kafka $kafkaMeta read $totalRecordsRead records through $numPolls
polls (polled " +
- s" out $numRecordsPolled records), taking $totalTimeReadNanos nanos,
during time span of " +
- s"$walTime nanos."
+logInfo(log"From Kafka ${MDC(CONSUMER, kafkaMeta)} read " +
+ log"${MDC(TOTAL_RECORDS_READ, totalRecordsRead)} records through " +
+ log"${MDC(COUNT_POLL, numPolls)} polls " +
+ log"(polled out ${MDC(COUNT_RECORDS_POLL, numRecordsPolled)} records), "
+
Review Comment:
KAFKA_RECORDS_PULLED_COUNT
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47594] Connector module: Migrate logInfo with variables to structured logging framework [spark]
gengliangwang commented on code in PR #46022:
URL: https://github.com/apache/spark/pull/46022#discussion_r1566459174
##
connector/kafka-0-10-sql/src/main/scala/org/apache/spark/sql/kafka010/consumer/KafkaDataConsumer.scala:
##
@@ -391,10 +392,12 @@ private[kafka010] class KafkaDataConsumer(
.getOrElse("")
val walTime = System.nanoTime() - startTimestampNano
-logInfo(
- s"From Kafka $kafkaMeta read $totalRecordsRead records through $numPolls
polls (polled " +
- s" out $numRecordsPolled records), taking $totalTimeReadNanos nanos,
during time span of " +
- s"$walTime nanos."
+logInfo(log"From Kafka ${MDC(CONSUMER, kafkaMeta)} read " +
+ log"${MDC(TOTAL_RECORDS_READ, totalRecordsRead)} records through " +
+ log"${MDC(COUNT_POLL, numPolls)} polls " +
Review Comment:
KAFKA_PULLS_COUNT?
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47594] Connector module: Migrate logInfo with variables to structured logging framework [spark]
gengliangwang commented on code in PR #46022:
URL: https://github.com/apache/spark/pull/46022#discussion_r1566451471
##
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectStreamingQueryCache.scala:
##
@@ -70,7 +70,9 @@ private[connect] class SparkConnectStreamingQueryCache(
log"Query Id: ${MDC(QUERY_ID, query.id)}.Existing value
${MDC(OLD_VALUE, existing)}, " +
log"new value ${MDC(NEW_VALUE, value)}.")
case None =>
- logInfo(s"Adding new query to the cache. Query Id ${query.id}, value
$value.")
+ logInfo(
+log"Adding new query to the cache. Query Id ${MDC(QUERY_ID,
query.id)}, " +
+ log"value ${MDC(QUERY_CACHE, value)}.")
Review Comment:
```suggestion
log"value ${MDC(QUERY_CACHE_VALUE, value)}.")
```
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47594] Connector module: Migrate logInfo with variables to structured logging framework [spark]
gengliangwang commented on code in PR #46022:
URL: https://github.com/apache/spark/pull/46022#discussion_r1566449680
##
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectServer.scala:
##
@@ -38,8 +39,8 @@ object SparkConnectServer extends Logging {
SparkConnectService.server.getListenSockets.asScala.foreach { sa =>
val isa = sa.asInstanceOf[InetSocketAddress]
logInfo(
-s"Spark Connect server started at: " +
- s"${isa.getAddress.getHostAddress}:${isa.getPort}")
+log"Spark Connect server started at: " +
+ log"${MDC(RPC_ADDRESS,
isa.getAddress.getHostAddress)}:${MDC(PORT, isa.getPort)}")
Review Comment:
Either way seems fine. Or, we can consider unifying them.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47594] Connector module: Migrate logInfo with variables to structured logging framework [spark]
gengliangwang commented on code in PR #46022:
URL: https://github.com/apache/spark/pull/46022#discussion_r1566448773
##
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectExecutionManager.scala:
##
@@ -95,7 +95,7 @@ private[connect] class SparkConnectExecutionManager() extends
Logging {
sessionHolder.addExecuteHolder(executeHolder)
executions.put(executeHolder.key, executeHolder)
lastExecutionTimeMs = None
- logInfo(s"ExecuteHolder ${executeHolder.key} is created.")
+ logInfo(log"ExecuteHolder ${MDC(LogKey.EXECUTE_HOLDER_KEY,
executeHolder.key)} is created.")
Review Comment:
```suggestion
logInfo(log"ExecuteHolder ${MDC(LogKey.EXECUTE_KEY,
executeHolder.key)} is created.")
```
##
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectExecutionManager.scala:
##
@@ -122,7 +122,7 @@ private[connect] class SparkConnectExecutionManager()
extends Logging {
if (executions.isEmpty) {
lastExecutionTimeMs = Some(System.currentTimeMillis())
}
- logInfo(s"ExecuteHolder $key is removed.")
+ logInfo(log"ExecuteHolder ${MDC(LogKey.EXECUTE_HOLDER_KEY, key)} is
removed.")
Review Comment:
```suggestion
logInfo(log"ExecuteHolder ${MDC(LogKey.EXECUTE_KEY, key)} is removed.")
```
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47594] Connector module: Migrate logInfo with variables to structured logging framework [spark]
panbingkun commented on PR #46022: URL: https://github.com/apache/spark/pull/46022#issuecomment-2056317368 cc @gengliangwang -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
