Respect promises on pipelined responses #23317
Merged
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
jasontedor
added
:Distributed/Network
jasontedor
force-pushed
the
pipeline-promises
branch
2 times, most recently
from
a4a101e
to
7ebdd4d
Compare
When pipelined responses are sent to the pipeline handler for writing, they are not necessarily written immediately. They must be held in a priority queue until all responses preceding the given response are written. This means that when write is invoked on the handler, the promise that is attached to the write invocation will not necessarily be the promise associated with the responses that are written while the queue is drained. To address this, the promise associated with a pipelined response must be held with the response and then used when the channel context is actually written to. This was introduced when ensuring that the releasing promise is always chained through on write calls lest the releasing promise never be invoked. This leads to many failing test cases, so no new test cases are needed here.
jasontedor
force-pushed
the
pipeline-promises
branch
from
7ebdd4d
to
1430c5f
Compare
This commit sets the intial size of the pipeline handler queue small to prevent waste if pipelined requests are never sent. Since the queue will grow quickly if pipeline requests are indeed set, this should not be problematic.
bleskes
approved these changes
Feb 23, 2017
There was a problem hiding this comment.
LGTM, though I don't feel comfortable I can oversee unexpected side effects (thanks for slowly making me learn netty). Maybe @spinscale can look at it as well?
| @@ -33,7 +33,8 @@ | |||
| */ | |||
| public class HttpPipeliningHandler extends ChannelDuplexHandler { | |||
|
|
|||
| private static final int INITIAL_EVENTS_HELD = 8; | |||
There was a problem hiding this comment.
Is this related to this change? if not , should we move it to separate PR?
There was a problem hiding this comment.
Sure, I'll move it to a separate change.
| * responses that precede it in the pipeline are written first. Note that the promise from the method invocation is | ||
| * not ignored, it will already be attached to an existing response and consumed when that response is drained. | ||
| */ | ||
| ctx.write(response.response(), response.promise()); |
There was a problem hiding this comment.
Should we assert at the beginning of this if clause that the promise in the HttpPipelinedResponse is that one we got as a parameter?
|
looks good to me |
This reverts commit 30cdefc.
* master: Align REST specs for HEAD requests Remove unnecessary result sorting in SearchPhaseController (elastic#23321) Fix SamplerAggregatorTests to have stable and predictable docIds Tests: Ensure multi node integ tests wait on first node
jasontedor
added a commit
that referenced
this pull request
Feb 23, 2017
When pipelined responses are sent to the pipeline handler for writing, they are not necessarily written immediately. They must be held in a priority queue until all responses preceding the given response are written. This means that when write is invoked on the handler, the promise that is attached to the write invocation will not necessarily be the promise associated with the responses that are written while the queue is drained. To address this, the promise associated with a pipelined response must be held with the response and then used when the channel context is actually written to. This was introduced when ensuring that the releasing promise is always chained through on write calls lest the releasing promise never be invoked. This leads to many failing test cases, so no new test cases are needed here. Relates #23317
jasontedor
added a commit
that referenced
this pull request
Feb 23, 2017
When pipelined responses are sent to the pipeline handler for writing, they are not necessarily written immediately. They must be held in a priority queue until all responses preceding the given response are written. This means that when write is invoked on the handler, the promise that is attached to the write invocation will not necessarily be the promise associated with the responses that are written while the queue is drained. To address this, the promise associated with a pipelined response must be held with the response and then used when the channel context is actually written to. This was introduced when ensuring that the releasing promise is always chained through on write calls lest the releasing promise never be invoked. This leads to many failing test cases, so no new test cases are needed here. Relates #23317
jasontedor
added a commit
that referenced
this pull request
Feb 23, 2017
When pipelined responses are sent to the pipeline handler for writing, they are not necessarily written immediately. They must be held in a priority queue until all responses preceding the given response are written. This means that when write is invoked on the handler, the promise that is attached to the write invocation will not necessarily be the promise associated with the responses that are written while the queue is drained. To address this, the promise associated with a pipelined response must be held with the response and then used when the channel context is actually written to. This was introduced when ensuring that the releasing promise is always chained through on write calls lest the releasing promise never be invoked. This leads to many failing test cases, so no new test cases are needed here. Relates #23317
|
Thanks for reviewing @bleskes and @spinscale. |
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.
When pipelined responses are sent to the pipeline handler for writing, they are not necessarily written immediately. They must be held in a priority queue until all responses preceding the given response are written. This means that when write is invoked on the handler, the promise that is attached to the write invocation will not necessarily be the promise associated with the responses that are written while the queue is drained. To address this, the promise associated with a pipelined response must be held with the response and then used when the channel context is actually written to. This was introduced when ensuring that the releasing promise is always chained through on write calls lest the releasing promise never be invoked. This leads to many failing test cases, so no new test cases are needed here.
Relates #23310, closes #23322