Simplify Netty4HttpPipeliningHandler #86791
Conversation
No need to collect responses into an intermediary list, we can write them directly as they become writable. Also, we can be more efficient for the common case where the response written is of the right sequence number and write it out directly.
original-brownbear
added
>non-issue
:Distributed Coordination/Network
elasticmachine
added
the
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
| // response is at the current sequence number and does not need to wait for any other response to be written so we write | ||
| // it out directly | ||
| doWrite(ctx, response, promise); | ||
| success = true; |
There was a problem hiding this comment.
Should this be set after the loop below?
There was a problem hiding this comment.
No in fact I consciously moved it here. Once we resolve the promise in doWrite (which we will do one way or another) we must not touch it again, that would always be a bug.
There was a problem hiding this comment.
Can we add a test that provokes that failure and then see that it is fixed by this PR? Seems we do not have tests that verify that we do not double notify failures and we used to sometimes do so?
There was a problem hiding this comment.
I don't think we can test this without creating an unrealistic situation. We should never be throwing here unless we exceed the outbound queue capacity to begin with. I really just fixed this as a cleanup to make it more obviously correct, unless we have a bug somewhere this doesn't change anything.
| if (response.getSequence() != writeSequence) { | ||
| if (outboundHoldingQueue.size() >= maxEventsHeld) { | ||
| int eventCount = outboundHoldingQueue.size() + 1; | ||
| throw new IllegalStateException( |
There was a problem hiding this comment.
This PR slightly changes semantics for when the queue is nearly full in that if the event that triggers that the queue is full has the right seq-no, it no longer fails. That sounds fine to me, but wanted to ensure other reviewers consider it.
| // response is at the current sequence number and does not need to wait for any other response to be written so we write | ||
| // it out directly | ||
| doWrite(ctx, response, promise); | ||
| success = true; |
There was a problem hiding this comment.
Can we add a test that provokes that failure and then see that it is fixed by this PR? Seems we do not have tests that verify that we do not double notify failures and we used to sometimes do so?
...ransport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java
Show resolved
Hide resolved
|
Thanks Ievgen + Henning! |
4 participants
No need to collect responses into an intermediary list, we can write them directly
as they become writable.
Also, we can be more efficient for the common case where the response written is of the
right sequence number and write it out directly.
follow up to #86133