Re: [jclouds/jclouds] fix jclouds/apis/swift retryOnRenew (#1027)
Merged at [master](https://git1-us-west.apache.org/repos/asf?p=jclouds.git;a=commit;h=9b3e6d91bff99b39699a333eec798d1f2cd5cded) and [1.9.x](https://git1-us-west.apache.org/repos/asf?p=jclouds.git;a=commit;h=9b3e6d91bff99b39699a333eec798d1f2cd5cded) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1027#issuecomment-257307273
Re: [jclouds/jclouds] fix jclouds/apis/swift retryOnRenew (#1027)
Closed #1027. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1027#event-841967206
Re: [jclouds/jclouds] fix jclouds/apis/swift retryOnRenew (#1027)
ok cool. I think I will volunteer for a new 1.9.x release :) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1027#issuecomment-256953771
Re: [jclouds/jclouds] fix jclouds/apis/swift retryOnRenew (#1027)
Absolutely. There are no plans to release 1.9.x, and it's been a while since we stopped backporting fixes. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1027#issuecomment-256953114
Re: [jclouds/jclouds] fix jclouds/apis/swift retryOnRenew (#1027)
merged at [1.9.x](http://git-wip-us.apache.org/repos/asf/jclouds/commit/7167b473) @nacx @zack-shoylev shall we port that to `master` as well? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1027#issuecomment-256952293
Re: [jclouds/jclouds] fix jclouds/apis/swift retryOnRenew (#1027)
nacx approved this pull request. lgtm! -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1027#pullrequestreview-6243053
Re: [jclouds/jclouds] fix jclouds/apis/swift retryOnRenew (#1027)
@nacx I have had some issues reproducing 1176. But I think this is alright to merge even if it does not fix it right away (but I strongly suspect it will). -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1027#issuecomment-256764753
Re: [jclouds/jclouds] fix jclouds/apis/swift retryOnRenew (#1027)
@zack-shoylev Could you give a try to this PR and check if this fixes JCLOUDS-1176? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1027#issuecomment-256586741
Re: [jclouds/jclouds] fix jclouds/apis/swift retryOnRenew (#1027)
Good change! Thanks, @andreaturli -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1027#issuecomment-256479800
Re: [jclouds/jclouds] fix jclouds/apis/swift retryOnRenew (#1027)
test408ShouldRetry for 2.0 should also be updated, it seems? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1027#issuecomment-256479071
Re: [jclouds/jclouds] fix jclouds/apis/swift retryOnRenew (#1027)
This might affect https://issues.apache.org/jira/browse/JCLOUDS-1176 -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1027#issuecomment-256475005
Re: [jclouds/jclouds] fix jclouds/apis/swift retryOnRenew (#1027)
> Can you expand on the problem this solves? Upon failed requests (response code > 3xx), the [BaseHttpCommandExecutorService](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/internal/BaseHttpCommandExecutorService.java#L134-L146) decides if the request should be retried or not by asking the configured retry handler. If the retry handler says no, then the request is considered failed and passed to the error handler so a proper exception is propagated. Otherwise, a new request with the same data is issued. Before [this commit](https://github.com/jclouds/jclouds/commit/ec63b55a04afdd5a970d8b05674deb4c3802c59c), error handlers and retry handlers were responsible of closing the payload, which was not good, as both implementations needed to be aware of each other. That commit just makes the caller release the payload when needed, so in general retry and error handlers no longer need to release it. The `shouldContinue` method in the `BaseHttpCommandExecutorService` will always release the payload when needed. The only thing to take into account is that retry handlers that need to read the payload to determine if the request should be retried *need* to buffer the payload or do whatever it takes (if the payload is not replayable) to make sure the error handler will be able to read the payload again from the beginning, in case the response is considered failed. The problem this issue fixes is that in some circumstances, the error handler got a closed stream, since the retry handler closed it (this only appeared when the jclouds wire logs were disabled, since payloads are saved when using wire logs). In summary, retry and error handlers should not release the payload. If retry handlers need to read it, they need to make sure the response is updated with a payload that can be read again from the beginning. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1027#issuecomment-256451963
Re: [jclouds/jclouds] fix jclouds/apis/swift retryOnRenew (#1027)
rebuild please -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1027#issuecomment-256448373
Re: [jclouds/jclouds] fix jclouds/apis/swift retryOnRenew (#1027)
@andrewgaul retriers in general shouldn't close the streams if they are not reading the payload, as https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/internal/BaseHttpCommandExecutorService.java#L144 will close it anyway. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1027#issuecomment-256448031
Re: [jclouds/jclouds] fix jclouds/apis/swift retryOnRenew (#1027)
Easier to review when ignoring whitespace: https://github.com/jclouds/jclouds/pull/1027/files?w=1 @andreaturli Can you expand on the problem this solves? @zack-shoylev Can you review this since you wrote `RetryOnRenew`? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1027#issuecomment-256444745
Re: [jclouds/jclouds] fix jclouds/apis/swift retryOnRenew (#1027)
BTWE, there are a couple more `RetryOnRenew` classes that do the same thing. Mind removing the finally block from all them too? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1027#issuecomment-256376952
Re: [jclouds/jclouds] fix jclouds/apis/swift retryOnRenew (#1027)
I think you are right! -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1027#issuecomment-256376606
Re: [jclouds/jclouds] fix jclouds/apis/swift retryOnRenew (#1027)
nacx commented on this pull request.
> @@ -116,7 +115,11 @@ public boolean shouldRetryRequest(HttpCommand command,
> HttpResponse response) {
return retry;
} finally {
- releasePayload(response);
+ // If the request is failed and is not going to be retried, the
+ // ErrorHandler will be invoked and it might need to read the payload.
+ // For some kind of payload sources, such as the OkHttp Source, if the
+ // payload is released, the upcoming operations will fail.
+ closeClientButKeepContentStream(response);
Is the retry handler doing something with the payload? If not, I'd just remove
the finally block to avoid buffering it in memory unnecessarily.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1027#pullrequestreview-5875597
[jclouds/jclouds] fix jclouds/apis/swift retryOnRenew (#1027)
/cc @nacx You can view, comment on, or merge this pull request online at: https://github.com/jclouds/jclouds/pull/1027 -- Commit Summary -- * fix jclouds/apis/swift retryOnRenew -- File Changes -- M common/openstack/src/main/java/org/jclouds/openstack/handlers/RetryOnRenew.java (7) -- Patch Links -- https://github.com/jclouds/jclouds/pull/1027.patch https://github.com/jclouds/jclouds/pull/1027.diff -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1027
