[SPARK-45377][CORE] Handle InputStream in NettyLogger #43165
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hasnain-db
changed the title
|
cc: @mridulm @JoshRosen this is now ready to review and has all green tests on CI |
mridulm
reviewed
Sep 30, 2023
common/network-common/src/main/java/org/apache/spark/network/util/NettyLogger.java
Outdated
Show resolved
Hide resolved
common/network-common/src/main/java/org/apache/spark/network/util/NettyLogger.java
Outdated
Show resolved
Hide resolved
srowen
reviewed
Oct 1, 2023
common/network-common/src/main/java/org/apache/spark/network/util/NettyLogger.java
Outdated
Show resolved
Hide resolved
hasnain-db
force-pushed
the
spark-tls-netty-logger
branch
from
f587591
to
15616e9
Compare
mridulm
approved these changes
Oct 1, 2023
common/network-common/src/main/java/org/apache/spark/network/util/NettyLogger.java
Outdated
Show resolved
Hide resolved
|
Merged to master |
RexXiong
pushed a commit
to apache/celeborn
that referenced
this pull request
Mar 28, 2024
### What changes were proposed in this pull request? Support Netty level logging at the network layer for Celeborn. To configure Netty level logging a LogHandler must be added to the channel pipeline. `NettyLogger` is introduced as a new class which is able to construct a log handler depending on the log level: - In case of `<Logger name="org.apache.celeborn.common.network.util.NettyLogger" level="DEBUG" additivity="false">`: a custom log handler is created which does not dump the message contents. This way the log is a bit more compact. Moreover when network level encryption is switched on this level might be sufficient. - In case of `<Logger name="org.apache.celeborn.common.network.util.NettyLogger" level="TRACE" additivity="false">`: Netty's own log handler is used which dumps the message contents. - Otherwise (when the logger is not `TRACE` or `DEBUG`) the pipeline does not contain a log handler (there is no runtime penalty for the default setting but a long running service must be restarted along with the new log level to have an effect). Backport: - [[SPARK-36719][CORE] Supporting Netty Logging at the network layer](apache/spark#33962) - [[SPARK-45377][CORE] Handle InputStream in NettyLogger](apache/spark#43165) ### Why are the changes needed? This level of logging proved to be sufficient during debugging some external shuffle related problem. Compared with the tcpdump this log lines can be more easily correlated with the Celeborn internal calls. Moreover the log layout can be configured to contain the thread names that way for a timeout a busy thread could be identified. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Local manually test. Closes #2423 from SteNicholas/CELEBORN-1359. Authored-by: SteNicholas <programgeek@163.com> Signed-off-by: Shuang <lvshuang.xjs@alibaba-inc.com>
CodingCat
pushed a commit
to CodingCat/incubator-celeborn
that referenced
this pull request
Apr 1, 2024
### What changes were proposed in this pull request? Support Netty level logging at the network layer for Celeborn. To configure Netty level logging a LogHandler must be added to the channel pipeline. `NettyLogger` is introduced as a new class which is able to construct a log handler depending on the log level: - In case of `<Logger name="org.apache.celeborn.common.network.util.NettyLogger" level="DEBUG" additivity="false">`: a custom log handler is created which does not dump the message contents. This way the log is a bit more compact. Moreover when network level encryption is switched on this level might be sufficient. - In case of `<Logger name="org.apache.celeborn.common.network.util.NettyLogger" level="TRACE" additivity="false">`: Netty's own log handler is used which dumps the message contents. - Otherwise (when the logger is not `TRACE` or `DEBUG`) the pipeline does not contain a log handler (there is no runtime penalty for the default setting but a long running service must be restarted along with the new log level to have an effect). Backport: - [[SPARK-36719][CORE] Supporting Netty Logging at the network layer](apache/spark#33962) - [[SPARK-45377][CORE] Handle InputStream in NettyLogger](apache/spark#43165) ### Why are the changes needed? This level of logging proved to be sufficient during debugging some external shuffle related problem. Compared with the tcpdump this log lines can be more easily correlated with the Celeborn internal calls. Moreover the log layout can be configured to contain the thread names that way for a timeout a busy thread could be identified. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Local manually test. Closes apache#2423 from SteNicholas/CELEBORN-1359. Authored-by: SteNicholas <programgeek@163.com> Signed-off-by: Shuang <lvshuang.xjs@alibaba-inc.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Handle
InputStreams in theNettyLoggerso we can print out how many available bytes there are.Why are the changes needed?
As part of the SSL support we are going to transfer
InputStreams via Netty, and this functionality makes it easy to see the size of the streams in the log at a glance.Does this PR introduce any user-facing change?
No
How was this patch tested?
CI. Tested as part of the changes in #42685 which this is split out of, I observed the logs there.
Was this patch authored or co-authored using generative AI tooling?
No