[SPARK-48209][CORE] Common (java side): Migrate `error/warn/info` with variables to structured logging framework by panbingkun · Pull Request #46493 · apache/spark

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

          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

        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

 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


 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

        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

          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

            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

      });
    } 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

    } 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

        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

  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


		public class LoggerFactory {

		public static Logger getLogger(String name) {

Conversation

panbingkun commented May 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

panbingkun commented May 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gengliangwang commented May 10, 2024

Uh oh!

panbingkun commented May 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gengliangwang commented May 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

panbingkun commented May 9, 2024 •

edited

Loading