You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "panbingkun (via GitHub)" <gi...@apache.org> on 2024/05/05 12:55:11 UTC
[PR] [SPARK-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
panbingkun opened a new pull request, #46390:
URL: https://github.com/apache/spark/pull/46390
### What changes were proposed in this pull request?
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
--
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-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #46390:
URL: https://github.com/apache/spark/pull/46390#discussion_r1591854610
##########
core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java:
##########
@@ -476,8 +479,9 @@ public void insertRecord(
assert(inMemSorter != null);
if (inMemSorter.numRecords() >= numElementsForSpillThreshold) {
- logger.info("Spilling data because number of spilledRecords crossed the threshold " +
- numElementsForSpillThreshold);
+ logger.info("Spilling data because number of spilledRecords crossed the threshold {}",
+ MDC.of(LogKeys.SHUFFLE_SPILL_NUM_ELEMENTS_FORCE_SPILL_THRESHOLD$.MODULE$,
Review Comment:
this key is sort of long. How about `NUM_ELEMENTS_SPILL_THRESHOLD`?
--
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-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #46390:
URL: https://github.com/apache/spark/pull/46390#discussion_r1590759422
##########
core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java:
##########
@@ -195,9 +197,9 @@ public long acquireExecutionMemory(long required, MemoryConsumer requestingConsu
}
consumers.add(requestingConsumer);
- if (logger.isDebugEnabled()) {
- logger.debug("Task {} acquired {} for {}", taskAttemptId, Utils.bytesToString(got),
- requestingConsumer);
+ if (LOGGER.isDebugEnabled()) {
+ LOGGER.debug("Task {} acquired {} for {}", String.valueOf(taskAttemptId),
+ Utils.bytesToString(got), requestingConsumer.toString());
Review Comment:
This place does not need to check whether `requestingConsumer` is null, because it has been checked previously as following:
<img width="501" alt="image" src="https://github.com/apache/spark/assets/15246973/e2284d90-ae20-48ec-92ff-4f91b4a111ae">
--
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-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #46390:
URL: https://github.com/apache/spark/pull/46390#discussion_r1590748268
##########
common/utils/src/main/java/org/apache/spark/internal/Logger.java:
##########
@@ -34,10 +34,26 @@ public class Logger {
this.slf4jLogger = slf4jLogger;
}
+ public boolean isErrorEnabled() {
+ return slf4jLogger.isErrorEnabled();
+ }
+
public void error(String msg) {
slf4jLogger.error(msg);
}
+ public void error(String msg, String arg) {
+ slf4jLogger.error(msg, arg);
+ }
+
+ public void error(String msg, String arg1, String arg2) {
+ slf4jLogger.error(msg, arg1, arg2);
+ }
+
+ public void error(String msg, String... args) {
Review Comment:
Why is the method signature not `public void error(String msg, Object... args)`
Because it conflicts with `public void error(String msg, MDC... mdcs)`
When we want to use `error(String msg, MDC... mdcs)`, it will enter `error(String msg, Object... args)`
--
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-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #46390:
URL: https://github.com/apache/spark/pull/46390#discussion_r1590756776
##########
core/src/main/java/org/apache/spark/io/ReadAheadInputStream.java:
##########
@@ -42,7 +45,7 @@
*/
public class ReadAheadInputStream extends InputStream {
- private static final Logger logger = LoggerFactory.getLogger(ReadAheadInputStream.class);
+ private static final Logger LOGGER = LoggerFactory.getLogger(ReadAheadInputStream.class);
Review Comment:
Let's capitalize variables of type `static final`.
--
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-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #46390:
URL: https://github.com/apache/spark/pull/46390#discussion_r1591904228
##########
core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeSorterSpillReader.java:
##########
@@ -17,21 +17,22 @@
package org.apache.spark.util.collection.unsafe.sort;
+import java.io.*;
Review Comment:
Is it the `import order of the file header`, or from `org.slf4j.Logger & org.slf4j.LoggerFactory` to `org.apache.spark.internal.Logger &org.apache.spark.internal.LoggerFactory`
The later change is to prohibit the import of `org.slf4j.Logger & org.slf4j.LoggerFactory` in java code later, so as to prevent subsequent developers from introducing`org.slf4j.Logger & org.slf4j.LoggerFactory` again, thus writing inconsistent logic.
--
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-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #46390:
URL: https://github.com/apache/spark/pull/46390#discussion_r1590745356
##########
common/utils/src/main/java/org/apache/spark/internal/Logger.java:
##########
@@ -34,10 +34,26 @@ public class Logger {
this.slf4jLogger = slf4jLogger;
}
+ public boolean isErrorEnabled() {
Review Comment:
It is found that some logic uses `this method` during migration.
--
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-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #46390:
URL: https://github.com/apache/spark/pull/46390#discussion_r1590749736
##########
core/src/main/java/org/apache/spark/io/ReadAheadInputStream.java:
##########
@@ -13,13 +13,6 @@
*/
package org.apache.spark.io;
-import com.google.common.base.Preconditions;
Review Comment:
Taking this chance, let's adjust the `import order` of `java code`
--
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-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #46390:
URL: https://github.com/apache/spark/pull/46390#discussion_r1591388245
##########
core/src/main/java/org/apache/spark/io/ReadAheadInputStream.java:
##########
@@ -42,7 +45,7 @@
*/
public class ReadAheadInputStream extends InputStream {
- private static final Logger logger = LoggerFactory.getLogger(ReadAheadInputStream.class);
+ private static final Logger LOGGER = LoggerFactory.getLogger(ReadAheadInputStream.class);
Review Comment:
I would suggest avoiding such unnecessary change.
In https://www.slf4j.org/api/org/slf4j/Logger.html, the example code is using `logger` as well.
--
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-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #46390:
URL: https://github.com/apache/spark/pull/46390#discussion_r1591654185
##########
core/src/main/java/org/apache/spark/io/ReadAheadInputStream.java:
##########
@@ -205,7 +208,7 @@ private void closeUnderlyingInputStreamIfNecessary() {
try {
underlyingInputStream.close();
} catch (IOException e) {
- logger.warn(e.getMessage(), e);
+ LOGGER.warn("{}", e, MDC.of(LogKeys.REASON$.MODULE$, e.getMessage()));
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-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #46390:
URL: https://github.com/apache/spark/pull/46390#discussion_r1591851400
##########
core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeSorterSpillReader.java:
##########
@@ -17,21 +17,22 @@
package org.apache.spark.util.collection.unsafe.sort;
+import java.io.*;
Review Comment:
let revert the unnecessary changes in this file
##########
core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeSorterSpillReader.java:
##########
@@ -17,21 +17,22 @@
package org.apache.spark.util.collection.unsafe.sort;
+import java.io.*;
Review Comment:
let's revert the unnecessary changes in this file
--
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-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #46390:
URL: https://github.com/apache/spark/pull/46390#discussion_r1591907044
##########
core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeSorterSpillReader.java:
##########
@@ -17,21 +17,22 @@
package org.apache.spark.util.collection.unsafe.sort;
+import java.io.*;
Review Comment:
Since there is no log migration in this file, why we need to change the import order here?
##########
core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeSorterSpillReader.java:
##########
@@ -17,21 +17,22 @@
package org.apache.spark.util.collection.unsafe.sort;
+import java.io.*;
Review Comment:
Since there is no log migration in this file, why do we need to change the import order here?
--
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-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #46390:
URL: https://github.com/apache/spark/pull/46390#discussion_r1591654034
##########
core/src/main/java/org/apache/spark/io/ReadAheadInputStream.java:
##########
@@ -42,7 +45,7 @@
*/
public class ReadAheadInputStream extends InputStream {
- private static final Logger logger = LoggerFactory.getLogger(ReadAheadInputStream.class);
+ private static final Logger LOGGER = LoggerFactory.getLogger(ReadAheadInputStream.class);
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-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #46390:
URL: https://github.com/apache/spark/pull/46390#discussion_r1591437324
##########
core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java:
##########
@@ -216,11 +218,11 @@ public long spill(long size, MemoryConsumer trigger) throws IOException {
return 0L;
}
- logger.info("Thread {} spilling sort data of {} to disk ({} {} so far)",
- Thread.currentThread().getId(),
- Utils.bytesToString(getMemoryUsage()),
- spillWriters.size(),
- spillWriters.size() > 1 ? " times" : " time");
+ LOGGER.info("Thread {} spilling sort data of {} to disk ({} {} so far)",
+ MDC.of(LogKeys.THREAD_ID$.MODULE$, Thread.currentThread().getId()),
+ MDC.of(LogKeys.MEMORY_SIZE$.MODULE$, Utils.bytesToString(getMemoryUsage())),
+ MDC.of(LogKeys.NUM_SPILL_WRITERS$.MODULE$, spillWriters.size()),
+ MDC.of(LogKeys.TIME_UNIT$.MODULE$, spillWriters.size() > 1 ? " times" : " time"));
Review Comment:
SPILL_TIMES?
--
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-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang closed pull request #46390: [SPARK-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework
URL: https://github.com/apache/spark/pull/46390
--
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-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
Posted by "cxzl25 (via GitHub)" <gi...@apache.org>.
cxzl25 commented on code in PR #46390:
URL: https://github.com/apache/spark/pull/46390#discussion_r1728682909
##########
core/src/main/java/org/apache/spark/shuffle/sort/ShuffleExternalSorter.java:
##########
@@ -416,8 +419,8 @@ public void insertRecord(Object recordBase, long recordOffset, int length, int p
// for tests
assert(inMemSorter != null);
if (inMemSorter.numRecords() >= numElementsForSpillThreshold) {
- logger.info("Spilling data because number of spilledRecords crossed the threshold " +
- numElementsForSpillThreshold);
+ logger.info("Spilling data because number of spilledRecords crossed the threshold {}" +
Review Comment:
When re-implementing SPARK-27734, I found that there was something wrong with the logs here.
` {}" + MDC.of ` -> `{}", MDC.of`
--
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-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #46390:
URL: https://github.com/apache/spark/pull/46390#issuecomment-2095704061
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-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-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #46390:
URL: https://github.com/apache/spark/pull/46390#discussion_r1591354244
##########
core/src/main/java/org/apache/spark/io/ReadAheadInputStream.java:
##########
@@ -205,7 +208,7 @@ private void closeUnderlyingInputStreamIfNecessary() {
try {
underlyingInputStream.close();
} catch (IOException e) {
- logger.warn(e.getMessage(), e);
+ LOGGER.warn("{}", e, MDC.of(LogKeys.REASON$.MODULE$, e.getMessage()));
Review Comment:
REASON => ERROR?
--
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-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #46390:
URL: https://github.com/apache/spark/pull/46390#discussion_r1591674796
##########
core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java:
##########
@@ -216,11 +218,11 @@ public long spill(long size, MemoryConsumer trigger) throws IOException {
return 0L;
}
- logger.info("Thread {} spilling sort data of {} to disk ({} {} so far)",
- Thread.currentThread().getId(),
- Utils.bytesToString(getMemoryUsage()),
- spillWriters.size(),
- spillWriters.size() > 1 ? " times" : " time");
+ LOGGER.info("Thread {} spilling sort data of {} to disk ({} {} so far)",
+ MDC.of(LogKeys.THREAD_ID$.MODULE$, Thread.currentThread().getId()),
+ MDC.of(LogKeys.MEMORY_SIZE$.MODULE$, Utils.bytesToString(getMemoryUsage())),
+ MDC.of(LogKeys.NUM_SPILL_WRITERS$.MODULE$, spillWriters.size()),
+ MDC.of(LogKeys.TIME_UNIT$.MODULE$, spillWriters.size() > 1 ? " times" : " time"));
Review Comment:
Done
--
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-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #46390:
URL: https://github.com/apache/spark/pull/46390#issuecomment-2097183311
@gengliangwang
All done.
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-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on PR #46390:
URL: https://github.com/apache/spark/pull/46390#issuecomment-2099214104
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-48134][CORE] Spark core (java side): Migrate `error/warn/info` with variables to structured logging framework [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #46390:
URL: https://github.com/apache/spark/pull/46390#discussion_r1591909956
##########
core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeSorterSpillReader.java:
##########
@@ -17,21 +17,22 @@
package org.apache.spark.util.collection.unsafe.sort;
+import java.io.*;
Review Comment:
The import order has been restored.
I actually have an idea to align `the import order of java` with `the import order of scala`.
It's OK, I restored it, 😄
##########
core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java:
##########
@@ -476,8 +479,9 @@ public void insertRecord(
assert(inMemSorter != null);
if (inMemSorter.numRecords() >= numElementsForSpillThreshold) {
- logger.info("Spilling data because number of spilledRecords crossed the threshold " +
- numElementsForSpillThreshold);
+ logger.info("Spilling data because number of spilledRecords crossed the threshold {}",
+ MDC.of(LogKeys.SHUFFLE_SPILL_NUM_ELEMENTS_FORCE_SPILL_THRESHOLD$.MODULE$,
Review Comment:
Done
--
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