Re: [jclouds] JCLOUDS-137: Retry on HTTP 500 AtmosError 1040 (#285)
> 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)
> + 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)
> + }
> +
> + @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)
> +
> + @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)
> + 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)
> + * @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)
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)
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)
> + 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)
> + 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
Re: [jclouds] JCLOUDS-137: Retry on HTTP 500 AtmosError 1040 (#285)
> + 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)
> + 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)
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)
[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)
[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)
[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)
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)
@shrinandj relevant to your interests. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/285#issuecomment-34682671
[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
