[CELEBORN-1359] Support Netty Logging at the network layer #2423
Conversation
This file contains hidden or 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
Member
Author
|
cc @mridulm, @otterc, @FMX, @RexXiong, @waitinfuture. |
SteNicholas
force-pushed
the
CELEBORN-1359
branch
from
bff759c to
20d5a04
Compare
SteNicholas
changed the title
Member
|
Code change LGTM, please update the PR description, we are using LOG4J2 with
|
Member
Author
|
@pan3793, I have updated the description of this pull request. PTAL. |
mridulm
approved these changes
Mar 27, 2024
Contributor
mridulm
left a comment
mridulm
left a comment
There was a problem hiding this comment.
Thanks for this !
This was in my list here CELEBORN-1350 :-)
otterc
approved these changes
Mar 27, 2024
Contributor
otterc
left a comment
otterc
left a comment
There was a problem hiding this comment.
LGTM. Just have a small question
common/src/main/java/org/apache/celeborn/common/network/TransportContext.java
Show resolved
Hide resolved
cxzl25
reviewed
Mar 28, 2024
common/src/main/java/org/apache/celeborn/common/network/util/NettyLogger.java
Show resolved
Hide resolved
cxzl25
approved these changes
Mar 28, 2024
pan3793
approved these changes
Mar 28, 2024
RexXiong
approved these changes
Mar 28, 2024
Contributor
RexXiong
left a comment
RexXiong
left a comment
There was a problem hiding this comment.
LGTM, Thanks! Merge to main(v0.5.0)
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 <[email protected]> Signed-off-by: Shuang <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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?
Support Netty level logging at the network layer for Celeborn. To configure Netty level logging a LogHandler must be added to the channel pipeline.
NettyLoggeris introduced as a new class which is able to construct a log handler depending on the log level:<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.<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.TRACEorDEBUG) 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:
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.