Skip to content
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

[Core] Do not requeue all responses #2840

Merged
merged 2 commits into from
Oct 21, 2016
Merged

Conversation

gmmeyer
Copy link
Contributor

@gmmeyer gmmeyer commented Sep 13, 2016

What does this PR do?

Not all requests should be replayed. We already do not replay requests that are too large (#2418), this extends the logic to 400 responses and makes the list of bad response types into a constant (RESPONSES_TO_REJECT), so that it can easily be added to or pruned.

Motivation

It was causing some issues in certain setups that these were being requeued. Also, it's wasteful to requeue responses that we know will be rejected. And, it's confusing to pollute the logs with re-rejections of validly rejected requests.

Additional Questions

Rather than having a blacklist of responses, we might want to have a whitelist of responses: Only requeue responses that were rejected for network errors or server errors.

@gmmeyer gmmeyer modified the milestones: 5.10.0, 5.9.1 Sep 13, 2016
@masci masci modified the milestones: 5.9.1, 5.9.2 Oct 13, 2016
@olivielpeau olivielpeau self-assigned this Oct 17, 2016
@olivielpeau olivielpeau modified the milestones: 5.10.0, 5.9.2 Oct 17, 2016
Copy link
Member

@olivielpeau olivielpeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@gmmeyer gmmeyer merged commit b18a6d6 into master Oct 21, 2016
jcejohnson pushed a commit to EFXCIA/dd-agent that referenced this pull request Oct 24, 2016
* Stops requests with a 400 response from being replayed

* makes the list of responses into a constant
@gmmeyer gmmeyer deleted the greg/do-not-replay-some-requests branch November 9, 2016 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants