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
Simplify OutboundHandler #72442
Simplify OutboundHandler #72442
Conversation
The whole serializer class adds nothing but complexity. Simplified it away to make this code easier to read while saving some objects as well and making the slow warning serialize slightly nicer.
Pinging @elastic/es-distributed (Team:Distributed) |
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.
Couple more cleanup suggestions; also I'd prefer to call this a >refactoring
rather than a >non-issue
.
server/src/main/java/org/elasticsearch/transport/OutboundHandler.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/transport/OutboundHandler.java
Outdated
Show resolved
Hide resolved
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
private void internalSend(TcpChannel channel, BytesReference reference, @Nullable OutboundMessage message, | ||
ActionListener<Void> listener) { | ||
final long startTime = threadPool.relativeTimeInMillis(); | ||
channel.getChannelStats().markAccessed(startTime); |
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 just fixed a small bug here in that we used to mark the channel as accessed before doing the serialisation, so if the serialisation failed then we'd delay the next ping without having sent anything else over the wire. No longer a problem ✅
final long startTime = threadPool.relativeTimeInMillis(); | ||
channel.getChannelStats().markAccessed(startTime); | ||
final long messageSize = reference.length(); | ||
TransportLogger.logOutboundMessage(channel, reference); |
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 this also fixes a small bug in that the tracer will log outbound pings now. Could argue it either way, but given that we logged inbound pings already I think this makes sense ✅
Thanks David :) |
The whole serializer class adds nothing but complexity. Simplified it away to make this code easier to read while saving some objects as well and making the slow warning serialize slightly nicer.
The whole serializer class adds nothing but complexity. Simplified it away to make this code easier to read while saving some objects as well and making the slow warning serialize slightly nicer.
BytesReference reference = sendContext.get(); | ||
private void internalSend(TcpChannel channel, BytesReference reference, @Nullable OutboundMessage message, | ||
ActionListener<Void> listener) { | ||
final long startTime = threadPool.relativeTimeInMillis(); |
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.
This also apparently fixes a small bug, see https://discuss.elastic.co/t/org-elasticsearch-action-get-multigetshardresponse-extremly-high-took-value/294434.
The whole serializer class adds nothing but complexity. Simplified it away
to make this code easier to read while saving some objects as well and making
the slow warning serialize slightly cleaner to read.