Fix courier "request already started" bug #6651
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
| this._all = queue.slice(0); | ||
|
|
||
| // Send the initial fetch status | ||
| this._reportStatus(); | ||
|
|
||
| return super.start(); |
There was a problem hiding this comment.
Moving this may be a problem. I'm not 100% on that, but I seem to remember there being a very specific reason why the parent constructor wasn't invoked until after the queue was created. Gross as hell though, and I really should have left a comment about it in the code.
There was a problem hiding this comment.
Yeah, I went looking for the discussion we had about it, but I'm pretty sure it's just what I talked about in the last paragraph of my description:
It feels wrong to me that the request is marked "started" before it's start() method is done, but the only other solution I have in mind requires that only one courier fetch cycle can initialize at a time (a much more impactful change).
There was a problem hiding this comment.
Thankfully the AbstractRequest#start() method is pretty simple and really only exists for reporting purposes and to make sure that requests aren't being executed multiple times.
|
I updated the PR description to reference the ticket. |
|
This LGTM. Assigning to @epixa to merge |
elastic-jasper
added a commit
that referenced
this pull request
Mar 25, 2016
--------- **Commit 1:** [courier/segmentedRequest] mark the request "started" synchronously * Original sha: 8a49926 * Authored by spalger <spalger@users.noreply.github.com> on 2016-03-24T21:35:00Z **Commit 2:** [courier/fetch] detect aborted requests more agressively * Original sha: c12f18d * Authored by spalger <spalger@users.noreply.github.com> on 2016-03-24T21:35:46Z **Commit 3:** [courier/callClient] if there are no executable requests, do nothing * Original sha: 8fef03b * Authored by spalger <spalger@users.noreply.github.com> on 2016-03-24T21:36:18Z **Commit 4:** [courier/tests] updated test * Original sha: 985f997 * Authored by spalger <spalger@users.noreply.github.com> on 2016-03-24T22:25:31Z
This was referenced Mar 28, 2016
elastic-jasper
added a commit
that referenced
this pull request
Mar 28, 2016
--------- **Commit 1:** [courier/segmentedRequest] mark the request "started" synchronously * Original sha: 8a49926 * Authored by spalger <spalger@users.noreply.github.com> on 2016-03-24T21:35:00Z **Commit 2:** [courier/fetch] detect aborted requests more agressively * Original sha: c12f18d * Authored by spalger <spalger@users.noreply.github.com> on 2016-03-24T21:35:46Z **Commit 3:** [courier/callClient] if there are no executable requests, do nothing * Original sha: 8fef03b * Authored by spalger <spalger@users.noreply.github.com> on 2016-03-24T21:36:18Z **Commit 4:** [courier/tests] updated test * Original sha: 985f997 * Authored by spalger <spalger@users.noreply.github.com> on 2016-03-24T22:25:31Z
epixa
added a commit
that referenced
this pull request
Mar 28, 2016
--------- **Commit 1:** [courier/segmentedRequest] mark the request "started" synchronously * Original sha: 8a49926 * Authored by spalger <spalger@users.noreply.github.com> on 2016-03-24T21:35:00Z **Commit 2:** [courier/fetch] detect aborted requests more agressively * Original sha: c12f18d * Authored by spalger <spalger@users.noreply.github.com> on 2016-03-24T21:35:46Z **Commit 3:** [courier/callClient] if there are no executable requests, do nothing * Original sha: 8fef03b * Authored by spalger <spalger@users.noreply.github.com> on 2016-03-24T21:36:18Z **Commit 4:** [courier/tests] updated test * Original sha: 985f997 * Authored by spalger <spalger@users.noreply.github.com> on 2016-03-24T22:25:31Z
elastic-jasper
added a commit
that referenced
this pull request
Mar 28, 2016
--------- **Commit 1:** [courier/segmentedRequest] mark the request "started" synchronously * Original sha: 8a49926 * Authored by spalger <spalger@users.noreply.github.com> on 2016-03-24T21:35:00Z **Commit 2:** [courier/fetch] detect aborted requests more agressively * Original sha: c12f18d * Authored by spalger <spalger@users.noreply.github.com> on 2016-03-24T21:35:46Z **Commit 3:** [courier/callClient] if there are no executable requests, do nothing * Original sha: 8fef03b * Authored by spalger <spalger@users.noreply.github.com> on 2016-03-24T21:36:18Z **Commit 4:** [courier/tests] updated test * Original sha: 985f997 * Authored by spalger <spalger@users.noreply.github.com> on 2016-03-24T22:25:31Z
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.
The current implementation of the
SegmentedRequestdoesn't mark the request started until after thefieldStatsapi provides an index list.This means that if two courier fetch cycles start in close succession then the same request will be included in both, causing a "Unable to start request because it has already started" error to be thrown.
This protects against that by calling the
super.start()method synchronously and then asynchronously populating the index queue. It feels wrong to me that the request is marked "started" before it'sstart()method is done, but the only other solution I have in mind requires that only one courier fetch cycle can initialize at a time (a much more impactful change).Fixes #5828