-
Notifications
You must be signed in to change notification settings - Fork 25.4k
Fix NullPointerException
in transport trace logger
#132243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also fix the NPE you found. I see no reason to even call openOrGetStreamInput()
within this format
method.
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
Yes, I also removed unused |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, could we also have a test covering this case in org.elasticsearch.transport.TransportLoggerTests
?
Sure, I am going to add a test case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Original report: IssueWhen trace-level logging is enabled, a node might disconnect from cluster due to an NPE that causes the transport connection closed between the data node and the master node. InboundMessages printed by
ResolveWe should only print warn logs instead of throwing NPE to caused connection closed between nodes. So try to catch all the exceptions instead of |
NullPointerException
in transport trace logger
When trace-level logging is enabled, a node might disconnect from
cluster due to an NPE that causes the transport connection closed
between the data node and the master node.
InboundMessages printed by
TransportLogger
might throw an NPE in theformat function because
content
might be NULL if another node sends anabnormal exception response.
Also there's no good reason to close the connection because of a logging
exception, so with this commit we catch all exceptions (rather than just
IOException
)