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

No easy way of preventing feign retries on POST/PUT requests #719

Closed
pasinski opened this issue Jun 12, 2018 · 2 comments
Closed

No easy way of preventing feign retries on POST/PUT requests #719

pasinski opened this issue Jun 12, 2018 · 2 comments
Labels
enhancement For recommending new capabilities

Comments

@pasinski
Copy link

pasinski commented Jun 12, 2018

I am using feign 9.5.1. In one interface I am doing POST, PUT and GET methods. I am OK with GET methods being retried on IOException coming from client (for instance, readTimeout etc.). Still I find it quite dangerous to retry POST/PUT methods.

In the Retryer interface I have no way of knowing what HTTP method was being used on this particular request (continueOrPropagate method accepts only RetryableException). Also, the method feign.FeignException#errorExecuting is static and for all IOException throws RetryableExcetpion. I haven't seen any easy way of altering this behaviour when studying the code of feign.SynchronousMethodHandler...

Response response;
        try {
            response = this.client.execute(request, this.options);
            response.toBuilder().request(request).build();
        } catch (IOException var15) {
            if(this.logLevel != Level.NONE) {
                this.logger.logIOException(this.metadata.configKey(), this.logLevel, var15, this.elapsedTime(start));
            }

            throw FeignException.errorExecuting(request, var15);
        }
  1. Making errorExecuting method of FeignException static and not allowing to impact for what requests to throw RetryableExcetpion is a poor design

  2. Putting into RetryableExcetion or FeignException information on what request lead to the exception might solve the problem, as the developer could have more control over when to retry. After all, information about the request is being passed to FeignException.errorExecuting method.

@kdavisk6
Copy link
Member

I agree. How about you take a shot at creating a PR for this. Let me know if you need any help or more suggestions.

@pasinski
Copy link
Author

pasinski commented Jun 28, 2018 via email

kdavisk6 added a commit to kdavisk6/feign that referenced this issue Jul 19, 2018
Closes OpenFeign#719

This change adds the original Request Method to `RetryableException`,
allowing implementers to determine if a retry should occur based on
method and exception type.

To support this, `Response` objects now require that the original
`Request` be present.  Test Cases, benchmarks, and documentation have
been added.
kdavisk6 added a commit to kdavisk6/feign that referenced this issue Jul 19, 2018
Closes OpenFeign#719

This change adds the original Request Method to `RetryableException`,
allowing implementers to determine if a retry should occur based on
method and exception type.

To support this, `Response` objects now require that the original
`Request` be present.  Test Cases, benchmarks, and documentation have
been added.
kdavisk6 added a commit to kdavisk6/feign that referenced this issue Jul 19, 2018
Closes OpenFeign#719

This change adds the original Request Method to `RetryableException`,
allowing implementers to determine if a retry should occur based on
method and exception type.

To support this, `Response` objects now require that the original
`Request` be present.  Test Cases, benchmarks, and documentation have
been added.
kdavisk6 added a commit to kdavisk6/feign that referenced this issue Jul 19, 2018
Closes OpenFeign#719

This change adds the original Request Method to `RetryableException`,
allowing implementers to determine if a retry should occur based on
method and exception type.

To support this, `Response` objects now require that the original
`Request` be present.  Test Cases, benchmarks, and documentation have
been added.
kdavisk6 added a commit to kdavisk6/feign that referenced this issue Jul 19, 2018
Closes OpenFeign#719

This change adds the original Request Method to `RetryableException`,
allowing implementers to determine if a retry should occur based on
method and exception type.

To support this, `Response` objects now require that the original
`Request` be present.  Test Cases, benchmarks, and documentation have
been added.
@kdavisk6 kdavisk6 added the enhancement For recommending new capabilities label Jul 20, 2018
kdavisk6 added a commit that referenced this issue Jul 24, 2018
Closes #719

This change adds the original Request Method to `RetryableException`,
allowing implementers to determine if a retry should occur based on
method and exception type.

To support this, `Response` objects now require that the original
`Request` be present.  Test Cases, benchmarks, and documentation have
been added.

* Refactored Request Method Attribute on Requests
* Added `HttpMethod` enum that represents the supported HTTP methods
replacing String handling.
* Deprecated `Request#method()` in favor of `Request#httpMethod()`
velo pushed a commit that referenced this issue Oct 7, 2024
Closes #719

This change adds the original Request Method to `RetryableException`,
allowing implementers to determine if a retry should occur based on
method and exception type.

To support this, `Response` objects now require that the original
`Request` be present.  Test Cases, benchmarks, and documentation have
been added.

* Refactored Request Method Attribute on Requests
* Added `HttpMethod` enum that represents the supported HTTP methods
replacing String handling.
* Deprecated `Request#method()` in favor of `Request#httpMethod()`
velo pushed a commit that referenced this issue Oct 8, 2024
Closes #719

This change adds the original Request Method to `RetryableException`,
allowing implementers to determine if a retry should occur based on
method and exception type.

To support this, `Response` objects now require that the original
`Request` be present.  Test Cases, benchmarks, and documentation have
been added.

* Refactored Request Method Attribute on Requests
* Added `HttpMethod` enum that represents the supported HTTP methods
replacing String handling.
* Deprecated `Request#method()` in favor of `Request#httpMethod()`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement For recommending new capabilities
Projects
None yet
Development

No branches or pull requests

2 participants