[SPARK-48209][CORE] Common (java side): Migrate `error/warn/info` with variables to structured logging framework #46493

panbingkun · 2024-05-09T02:54:33Z

What changes were proposed in this pull request?

The pr aims to
1.migrate error/warn/info in module common with variables to structured logging framework for java side.
2.convert all dependencies on org.slf4j.Logger & org.slf4j.LoggerFactory to org.apache.spark.internal.Logger & org.apache.spark.internal.LoggerFactory, in order to completely prohibit importing org.slf4j.Logger & org.slf4j.LoggerFactory in java code later.

Why are the changes needed?

To enhance Apache Spark's logging system by implementing structured logging.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass GA.

Was this patch authored or co-authored using generative AI tooling?

No.

panbingkun · 2024-05-10T01:23:57Z

common/network-common/src/main/java/org/apache/spark/network/ssl/SSLFactory.java

@@ -136,7 +135,7 @@ public void destroy() {
          try {
            manager.destroy();
          } catch (InterruptedException ex) {
-            logger.info("Interrupted while destroying trust manager: " + ex.toString(), ex);


Remove redundant ex.toString().

panbingkun · 2024-05-10T01:27:37Z

.../network-shuffle/src/main/java/org/apache/spark/network/shuffle/RetryingBlockTransferor.java

-        listener.getTransferType(), blockIdsToTransfer.length,
-        numRetries > 0 ? "(after " + numRetries + " retries)" : ""), e);
-
+      if (numRetries > 0) {


Print different log message according to the value of numRetries

panbingkun · 2024-05-10T01:41:34Z

...tor/connect/common/src/main/scala/org/apache/spark/sql/connect/client/GrpcRetryHandler.scala

@@ -200,7 +200,7 @@ private[sql] object GrpcRetryHandler extends Logging {
        if (time.isDefined) {
          logWarning(
            log"Non-Fatal error during RPC execution: ${MDC(ERROR, lastException)}, " +
-              log"retrying (wait=${MDC(WAIT_TIME, time.get.toMillis)} ms, " +


Unify WAIT_TIME into RETRY_WAIT_TIME

panbingkun · 2024-05-10T01:49:17Z

core/src/main/scala/org/apache/spark/network/netty/NettyBlockTransferService.scala

@@ -30,7 +30,7 @@ import com.codahale.metrics.{Metric, MetricSet}

 import org.apache.spark.{SecurityManager, SparkConf}
 import org.apache.spark.ExecutorDeadException
-import org.apache.spark.internal.config
+import org.apache.spark.internal.{config, LogKeys, MDC}


Because NettyBlockTransferService extends BlockTransferService
BlockTransferService is java code, and this change triggered it.

panbingkun · 2024-05-10T01:51:46Z

common/utils/src/main/java/org/apache/spark/internal/LoggerFactory.java

@@ -19,6 +19,11 @@

 public class LoggerFactory {

+  public static Logger getLogger(String name) {


YarnShuffleService will use it:

panbingkun · 2024-05-10T01:58:01Z

...ork-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolver.java

@@ -368,7 +382,8 @@ public int removeBlocks(String appId, String execId, String[] blockIds) {
      if (file.delete()) {
        numRemovedBlocks++;
      } else {
-        logger.warn("Failed to delete block: " + file.getAbsolutePath());
+        logger.warn("Failed to delete block: {}",


I'm not sure it's appropriate to call this LogKeys.PATH$.MODULE$?

yes, path is fine

panbingkun · 2024-05-10T02:00:51Z

...ork-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolver.java

@@ -472,7 +487,8 @@ static ConcurrentMap<AppExecId, ExecutorShuffleInfo> reloadRegisteredExecutors(D
            break;
          }
          AppExecId id = parseDbAppExecKey(key);
-          logger.info("Reloading registered executors: " +  id.toString());
+          logger.info("Reloading registered executors: {}",
+            MDC.of(LogKeys.APP_EXECUTOR_ID$.MODULE$, id.toString()));


Remove redundancy .toString()

panbingkun · 2024-05-10T02:09:07Z

common/network-common/src/main/java/org/apache/spark/network/ssl/ReloadingX509TrustManager.java

@@ -211,13 +210,13 @@ public void run() {
            this.reloadCount += 1;
          } catch (Exception ex) {
            logger.warn(
-              "Could not load truststore (keep using existing one) : " + ex.toString(),


Remove redundancy ex.toString()

panbingkun · 2024-05-10T02:20:19Z

...on/network-common/src/main/java/org/apache/spark/network/server/TransportRequestHandler.java

@@ -298,7 +303,9 @@ public void onFailure(Throwable e) {
      });
    } catch (Exception e) {
      logger.error("Error while invoking receiveMergeBlockMetaReq() for appId {} shuffleId {} "
-        + "reduceId {}", req.appId, req.shuffleId, req.appId, e);


fix typo
reduceId {req.appId} -> reduceId {req.reduceId}

panbingkun · 2024-05-10T02:38:10Z

common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleService.java

@@ -363,7 +367,8 @@ static MergedShuffleFileManager newMergedShuffleFileManagerInstance(
      return mergeManagerSubClazz.getConstructor(TransportConf.class, File.class)
        .newInstance(conf, mergeManagerFile);
    } catch (Exception e) {
-      defaultLogger.error("Unable to create an instance of {}", mergeManagerImplClassName);
+      defaultLogger.error("Unable to create an instance of {}",
+        MDC.of(LogKeys.CLASS_NAME$.MODULE$, mergeManagerImplClassName));


Or call MERGED_SHUFFLE_FILE_MANAGER ?

CLASS_NAME looks good.

panbingkun · 2024-05-10T02:51:28Z

.../network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java

@@ -1999,7 +2042,9 @@ private AppPathsInfo(
      this.subDirsPerLocalDir = subDirsPerLocalDir;
      if (logger.isInfoEnabled()) {
        logger.info("Updated active local dirs {} and sub dirs {} for application {}",
-          Arrays.toString(activeLocalDirs),subDirsPerLocalDir, appId);
+          MDC.of(LogKeys.LOCAL_DIRS$.MODULE$, Arrays.toString(activeLocalDirs)),
+          MDC.of(LogKeys.NUM_SUB_DIRS$.MODULE$, subDirsPerLocalDir),


The subDirsPerLocalDir is actually a number (the number of sub directories), not a sub directory path.

panbingkun · 2024-05-10T03:04:39Z

cc @gengliangwang

gengliangwang · 2024-05-10T21:38:47Z

...ork-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolver.java

-    logger.info("Application {} removed, cleanupLocalDirs = {}", appId, cleanupLocalDirs);
+    logger.info("Application {} removed, cleanupLocalDirs = {}",
+      MDC.of(LogKeys.APP_ID$.MODULE$, appId),
+      MDC.of(LogKeys.LOCAL_DIRS$.MODULE$, cleanupLocalDirs));


cleanupLocalDirs is a boolean. let's just use CLEANUP_LOCAL_DIRS

gengliangwang · 2024-05-10T22:24:01Z

LGTM overall. Thanks for the work!

panbingkun · 2024-05-10T23:22:26Z

LGTM overall. Thanks for the work!

Updated, thanks! ❤️

gengliangwang · 2024-05-10T23:49:18Z

common/utils/src/main/scala/org/apache/spark/internal/LogKey.scala

@@ -292,6 +301,7 @@ object LogKeys {
  case object LOADED_VERSION extends LogKey
  case object LOAD_FACTOR extends LogKey
  case object LOAD_TIME extends LogKey
+  case object LOCAL_DIRS extends LogKey


we don't need this anymore

Hmm ..., the following uses it, do we should merge it to PATH?

yes, merge into PATHS

gengliangwang · 2024-05-14T04:03:51Z

Thanks, merging to master

panbingkun added 7 commits May 8, 2024 13:35

part 1

a95d52e

Merge branch 'master' into common_java_sl

7c4a232

part 2

d782260

part 3

91ba4e9

part 4

1b734e1

update

beb3133

Merge branch 'master' into common_java_sl

e6e18ba

github-actions bot added SQL CORE CONNECT labels May 9, 2024

panbingkun added 5 commits May 9, 2024 11:28

update

916c811

update

afd6212

update

ea0cb5b

update

5d3e363

Merge branch 'master' into common_java_sl

8646c10

panbingkun commented May 10, 2024

View reviewed changes

update

77164ac

panbingkun marked this pull request as ready for review May 10, 2024 03:04

gengliangwang reviewed May 10, 2024

View reviewed changes

panbingkun added 2 commits May 11, 2024 07:07

Merge branch 'master' into common_java_sl

4832061

update

6cb6437

gengliangwang reviewed May 10, 2024

View reviewed changes

panbingkun added 2 commits May 14, 2024 05:57

Merge branch 'master' into common_java_sl

f5ee214

update

6d1fe87

gengliangwang approved these changes May 14, 2024

View reviewed changes

gengliangwang closed this in 78d2a86 May 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-48209][CORE] Common (java side): Migrate `error/warn/info` with variables to structured logging framework #46493

[SPARK-48209][CORE] Common (java side): Migrate `error/warn/info` with variables to structured logging framework #46493

panbingkun commented May 9, 2024 •

edited

panbingkun May 10, 2024

panbingkun May 10, 2024

panbingkun May 10, 2024

panbingkun May 10, 2024

panbingkun May 10, 2024

panbingkun May 10, 2024

gengliangwang May 10, 2024

panbingkun May 10, 2024

panbingkun May 10, 2024

panbingkun May 10, 2024

panbingkun May 10, 2024

gengliangwang May 10, 2024

panbingkun May 10, 2024

panbingkun commented May 10, 2024

gengliangwang May 10, 2024

panbingkun May 10, 2024

gengliangwang commented May 10, 2024

panbingkun commented May 10, 2024

gengliangwang May 10, 2024

panbingkun May 11, 2024

gengliangwang May 13, 2024

panbingkun May 14, 2024

gengliangwang commented May 14, 2024

		@@ -19,6 +19,11 @@

		public class LoggerFactory {

		public static Logger getLogger(String name) {

[SPARK-48209][CORE] Common (java side): Migrate error/warn/info with variables to structured logging framework #46493

[SPARK-48209][CORE] Common (java side): Migrate error/warn/info with variables to structured logging framework #46493

Conversation

panbingkun commented May 9, 2024 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panbingkun commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gengliangwang commented May 10, 2024

panbingkun commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gengliangwang commented May 14, 2024

[SPARK-48209][CORE] Common (java side): Migrate `error/warn/info` with variables to structured logging framework #46493

[SPARK-48209][CORE] Common (java side): Migrate `error/warn/info` with variables to structured logging framework #46493

panbingkun commented May 9, 2024 •

edited