You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Andrew Gaul <no...@github.com> on 2014/02/10 22:09:29 UTC
[jclouds] JCLOUDS-137: Retry on HTTP 500 AtmosError 1040 (#285)
You can merge this Pull Request by running:
git pull https://github.com/maginatics/jclouds atmos-500-1040
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/285
-- Commit Summary --
* JCLOUDS-137: Retry on HTTP 500 AtmosError 1040
-- File Changes --
M apis/atmos/src/main/java/org/jclouds/atmos/config/AtmosRestClientModule.java (2)
A apis/atmos/src/main/java/org/jclouds/atmos/handlers/AtmosServerErrorRetryHandler.java (85)
A apis/atmos/src/test/java/org/jclouds/atmos/handlers/AtmosServerErrorRetryHandlerTest.java (86)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/285.patch
https://github.com/jclouds/jclouds/pull/285.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/285
Re: [jclouds] JCLOUDS-137: Retry on HTTP 500 AtmosError 1040 (#285)
Posted by Andrew Gaul <no...@github.com>.
Spurious test failure, tracked by [JCLOUDS-429](https://issues.apache.org/jira/browse/JCLOUDS-429).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/285#issuecomment-34688336
Re: [jclouds] JCLOUDS-137: Retry on HTTP 500 AtmosError 1040 (#285)
Posted by Andrew Phillips <no...@github.com>.
> + return false;
> + }
> + if (response.getStatusCode() == 500) {
> + byte[] content = HttpUtils.closeClientButKeepContentStream(response);
> + // Content can be null in the case of HEAD requests
> + if (content != null) {
> + try {
> + AtmosError error = utils.parseAtmosErrorFromContent(command, response,
> + new String(content));
> + if (error.getCode() == 1040) { // The server is busy. Please try again.
> + return backoffHandler.shouldRetryRequest(command, response);
> + }
> + // don't increment count before here, since backoff handler does already
> + command.incrementFailureCount();
> + } catch (HttpException e) {
> + logger.warn(e, "error parsing response: %s", new String(content));
Do we need to increment the failure count in here? Or move the increment into a `finally` block?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/285/files#r9651130
Re: [jclouds] JCLOUDS-137: Retry on HTTP 500 AtmosError 1040 (#285)
Posted by Andrew Gaul <no...@github.com>.
Committed to master and 1.7.x.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/285#issuecomment-34800494
Re: [jclouds] JCLOUDS-137: Retry on HTTP 500 AtmosError 1040 (#285)
Posted by Andrew Gaul <no...@github.com>.
> + private int retryCountLimit = 5;
> + @Resource
> + protected Logger logger = Logger.NULL;
> +
> + public boolean shouldRetryRequest(HttpCommand command, HttpResponse response) {
> + if (command.getFailureCount() > retryCountLimit) {
> + return false;
> + }
> + if (response.getStatusCode() == 500) {
> + byte[] content = HttpUtils.closeClientButKeepContentStream(response);
> + // Content can be null in the case of HEAD requests
> + if (content != null) {
> + try {
> + AtmosError error = utils.parseAtmosErrorFromContent(command, response,
> + new String(content));
> + if (error.getCode() == 1040) { // The server is busy. Please try again.
I retabulated the 500 error codes and we could retry on most of these, although I have some concern since I have never experienced these errors:
Error Code | Error Message | HTTP Status Code and Description
---------- | ------------------------------------------------------------ | --------------------------------
1001 | The server encountered an internal error. Please try again. | 500 Internal Server Error
1007 | The server encountered an internal error. Please try again. | 500 Internal Server Error
1013 | The server encountered an internal error. Please try again. | 500 Internal Server Error
1019 | The server encountered an I/O error. Please try again. | 500 Internal Server Error
1020 | The requested resource is missing or could not be found. | 500 Internal Server Error
1024 | The server encountered an internal error. Please try again. | 500 Internal Server Error
1025 | The server encountered an internal error. Please try again. | 500 Internal Server Error
1026 | The server encountered an internal error. Please try again. | 500 Internal Server Error
1027 | The server encountered an internal error. Please try again. | 500 Internal Server Error
1028 | The server encountered an internal error. Please try again. | 500 Internal Server Error
1029 | The server encountered an internal error. Please try again. | 500 Internal Server Error
1040 | The server is busy. Please try again | 500 Internal Server Error
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/285/files#r9613290
Re: [jclouds] JCLOUDS-137: Retry on HTTP 500 AtmosError 1040 (#285)
Posted by BuildHive <no...@github.com>.
[jclouds ยป jclouds #827](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/827/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/285#issuecomment-34687590
Re: [jclouds] JCLOUDS-137: Retry on HTTP 500 AtmosError 1040 (#285)
Posted by Andrew Phillips <no...@github.com>.
> + * @author Andrew Gaul
> + */
> +public class AtmosServerErrorRetryHandler implements HttpRetryHandler {
> + private final AtmosUtils utils;
> + private final BackoffLimitedRetryHandler backoffHandler;
> +
> + @Inject
> + public AtmosServerErrorRetryHandler(BackoffLimitedRetryHandler backoffHandler,
> + AtmosUtils utils) {
> + this.backoffHandler = backoffHandler;
> + this.utils = utils;
> + }
> +
> + @Inject(optional = true)
> + @Named(Constants.PROPERTY_MAX_RETRIES)
> + private int retryCountLimit = 5;
Add a blank line after this, and move below the logger as it's `private`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/285/files#r9651071
Re: [jclouds] JCLOUDS-137: Retry on HTTP 500 AtmosError 1040 (#285)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #584](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/584/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/285#issuecomment-34687898
Re: [jclouds] JCLOUDS-137: Retry on HTTP 500 AtmosError 1040 (#285)
Posted by Andrew Gaul <no...@github.com>.
Closed #285.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/285
Re: [jclouds] JCLOUDS-137: Retry on HTTP 500 AtmosError 1040 (#285)
Posted by Andrew Gaul <no...@github.com>.
> + private int retryCountLimit = 5;
> + @Resource
> + protected Logger logger = Logger.NULL;
> +
> + public boolean shouldRetryRequest(HttpCommand command, HttpResponse response) {
> + if (command.getFailureCount() > retryCountLimit) {
> + return false;
> + }
> + if (response.getStatusCode() == 500) {
> + byte[] content = HttpUtils.closeClientButKeepContentStream(response);
> + // Content can be null in the case of HEAD requests
> + if (content != null) {
> + try {
> + AtmosError error = utils.parseAtmosErrorFromContent(command, response,
> + new String(content));
> + if (error.getCode() == 1040) { // The server is busy. Please try again.
Let's go with this commit as-is; we have a convenient table if we need to revisit this.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/285/files#r9641179
Re: [jclouds] JCLOUDS-137: Retry on HTTP 500 AtmosError 1040 (#285)
Posted by Shri Javadekar <no...@github.com>.
The turnaround time on this is INCREDIBLE!! :+1:
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/285#issuecomment-34685027
Re: [jclouds] JCLOUDS-137: Retry on HTTP 500 AtmosError 1040 (#285)
Posted by Andrew Phillips <no...@github.com>.
> +
> + @Inject
> + public AtmosServerErrorRetryHandler(BackoffLimitedRetryHandler backoffHandler,
> + AtmosUtils utils) {
> + this.backoffHandler = backoffHandler;
> + this.utils = utils;
> + }
> +
> + @Inject(optional = true)
> + @Named(Constants.PROPERTY_MAX_RETRIES)
> + private int retryCountLimit = 5;
> + @Resource
> + protected Logger logger = Logger.NULL;
> +
> + public boolean shouldRetryRequest(HttpCommand command, HttpResponse response) {
> + if (command.getFailureCount() > retryCountLimit) {
Check for `command.getFailureCount() > retryCountLimit || response.getStatusCode() != 500` here? Then we can skip the outermost `if` below and lose one level of indenting?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/285/files#r9651174
Re: [jclouds] JCLOUDS-137: Retry on HTTP 500 AtmosError 1040 (#285)
Posted by Andrew Phillips <no...@github.com>.
> + }
> +
> + @Inject(optional = true)
> + @Named(Constants.PROPERTY_MAX_RETRIES)
> + private int retryCountLimit = 5;
> + @Resource
> + protected Logger logger = Logger.NULL;
> +
> + public boolean shouldRetryRequest(HttpCommand command, HttpResponse response) {
> + if (command.getFailureCount() > retryCountLimit) {
> + return false;
> + }
> + if (response.getStatusCode() == 500) {
> + byte[] content = HttpUtils.closeClientButKeepContentStream(response);
> + // Content can be null in the case of HEAD requests
> + if (content != null) {
Invert this to a `if (content == null) { return false; }` block to lose another level of indenting?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/285/files#r9651204
Re: [jclouds] JCLOUDS-137: Retry on HTTP 500 AtmosError 1040 (#285)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1056](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1056/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/285#issuecomment-34687928
Re: [jclouds] JCLOUDS-137: Retry on HTTP 500 AtmosError 1040 (#285)
Posted by Shri Javadekar <no...@github.com>.
> + private int retryCountLimit = 5;
> + @Resource
> + protected Logger logger = Logger.NULL;
> +
> + public boolean shouldRetryRequest(HttpCommand command, HttpResponse response) {
> + if (command.getFailureCount() > retryCountLimit) {
> + return false;
> + }
> + if (response.getStatusCode() == 500) {
> + byte[] content = HttpUtils.closeClientButKeepContentStream(response);
> + // Content can be null in the case of HEAD requests
> + if (content != null) {
> + try {
> + AtmosError error = utils.parseAtmosErrorFromContent(command, response,
> + new String(content));
> + if (error.getCode() == 1040) { // The server is busy. Please try again.
As per the pdf document you referred above, when the status code is 500, the only error code for which we should *not* retry is 1020. For all others (including 1040), we should retry.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/285/files#r9610835
Re: [jclouds] JCLOUDS-137: Retry on HTTP 500 AtmosError 1040 (#285)
Posted by Andrew Phillips <no...@github.com>.
> Committed to master and 1.7.x.
Ah, I see my review is already a little late :-( @andrewgaul: most of the questions are minor, the only one that we may really need to look at, I think, is about making sure we increment the failure count correctly.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/285#issuecomment-34826715
Re: [jclouds] JCLOUDS-137: Retry on HTTP 500 AtmosError 1040 (#285)
Posted by Andrew Phillips <no...@github.com>.
> + HttpCommand command = createMock(HttpCommand.class);
> + String content = String.format(HTTP_MESSAGE_FORMAT, 1040, "The server is busy. Please try again");
> + HttpResponse response = HttpResponse.builder().statusCode(500).payload(content).build();
> +
> + expect(command.getFailureCount()).andReturn(0).once();
> + expect(utils.parseAtmosErrorFromContent(command, response, content)).andReturn(new AtmosError(1040, "The server is busy. Please try again")).once();
> + expect(backoffLimitedRetryHandler.shouldRetryRequest(command, response)).andReturn(true).once();
> +
> + replay(utils, backoffLimitedRetryHandler, command);
> +
> + AtmosServerErrorRetryHandler retry = new AtmosServerErrorRetryHandler(backoffLimitedRetryHandler, utils);
> +
> + assertTrue(retry.shouldRetryRequest(command, response));
> +
> + verify(utils, backoffLimitedRetryHandler, command);
> + }
Do we also need tests here for non-1040 error code and for `null` content?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/285/files#r9651227
Re: [jclouds] JCLOUDS-137: Retry on HTTP 500 AtmosError 1040 (#285)
Posted by Andrew Gaul <no...@github.com>.
@shrinandj relevant to your interests.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/285#issuecomment-34682671
Re: [jclouds] JCLOUDS-137: Retry on HTTP 500 AtmosError 1040 (#285)
Posted by Shri Javadekar <no...@github.com>.
> + private int retryCountLimit = 5;
> + @Resource
> + protected Logger logger = Logger.NULL;
> +
> + public boolean shouldRetryRequest(HttpCommand command, HttpResponse response) {
> + if (command.getFailureCount() > retryCountLimit) {
> + return false;
> + }
> + if (response.getStatusCode() == 500) {
> + byte[] content = HttpUtils.closeClientButKeepContentStream(response);
> + // Content can be null in the case of HEAD requests
> + if (content != null) {
> + try {
> + AtmosError error = utils.parseAtmosErrorFromContent(command, response,
> + new String(content));
> + if (error.getCode() == 1040) { // The server is busy. Please try again.
I haven't seen these failures myself. I'm fine with the current change if you're not comfortable with retrying on all returned error codes except 1020.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/285/files#r9637835