Adding Method to Retryable Exception for evaluation #744

kdavisk6 · 2018-07-19T18:55:14Z

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.

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.

velo

I add a few pointer... overall looks good.

velo · 2018-07-19T21:22:01Z

core/src/main/java/feign/Client.java

@@ -65,7 +65,7 @@ public Default(SSLSocketFactory sslContextFactory, HostnameVerifier hostnameVeri
    @Override
    public Response execute(Request request, Options options) throws IOException {
      HttpURLConnection connection = convertAndSend(request, options);
-      return convertResponse(connection).toBuilder().request(request).build();
+      return convertResponse(connection, request).toBuilder().request(request).build();


By doing this you can eliminate the .toBuilder().request(request).build()

I must have missed this one during testing. I'll remove it.

velo · 2018-07-19T21:23:23Z

core/src/main/java/feign/Request.java

@@ -37,15 +41,15 @@ public static Request create(String method,
    return new Request(method, url, headers, body, charset);
  }

-  private final String method;
+  private final Methods method;


Nice... maybe just call it Method? Or HttpMethod to avoid conflict with other methods =D

I had not considered that, good catch! I'll rename to HttpMethod

velo · 2018-07-19T21:24:34Z

core/src/main/java/feign/Response.java

@@ -45,11 +45,12 @@

  private Response(Builder builder) {
    checkState(builder.status >= 200, "Invalid status code: %s", builder.status);
+    checkState(builder.request != null, "original request is required");


why not use checkNotNull?

I wanted an IllegalStateException thrown and not a NullPointerException. From my point of view, I prefer not to throw NPE's directly. But I'm open to change it if you feel strongly enough.

velo · 2018-07-19T21:31:43Z

core/src/main/java/feign/Response.java

+      checkNotNull(request, "the original request is required on all responses");
+
+      /* don't keep the body, we don't want to tie up memory on large requests */
+      this.request = Request.create(


IMO, Request and Response object are short lived, so, feels like any gain we would have memory usage by creating a new request, we would loose on GC time for 2 objects....

If someone decides to keep the Response in memory for some reason, I think, they would like to have the whole Request.

Lemme know your thoughts

My concern is with larger requests that deal with multi-part uploads and pure binary requests. Keeping the body in memory after the request feels like it may lead to unexpected memory usages and leaks. I had not considered the GC impact.

It feels like an early optimization for me.

What if we include the original Request for now, and try to fix the problem when we spot real use cases?

Understood. I'll change to to reference the original and not create a new request.

velo · 2018-07-19T21:32:12Z

core/src/main/java/feign/RetryableException.java

@@ -24,20 +24,23 @@
  private static final long serialVersionUID = 1L;

  private final Long retryAfter;
+  private final String method;


What about using the enumeration Methods?

velo · 2018-07-19T21:36:05Z

core/src/main/java/feign/Request.java

@@ -54,7 +58,7 @@ public static Request create(String method,

  /* Method to invoke on the server. */
  public String method() {


I thin we should create a new method httpMethod() that will return the enum and then deprecate this one to be removed on feign 12!?

Yeah, let's do that. I had not added the Enum to RetryableException due to this method. I'll make that change here and in RetryableException

velo · 2018-07-19T22:22:10Z

core/src/test/java/feign/client/DefaultClientTest.java

@@ -13,6 +13,7 @@
 */
 package feign.client;

+import feign.FeignException;


Think that is not necessary

I'll clean that up.

kdavisk6 · 2018-07-20T16:24:29Z

@velo, remarks addressed and the PR has been updated.

* Added `HttpMethod` enum that represents the supported HTTP methods replacing String handling. * Deprecated `Request#method()` in favor of `Request#httpMethod()`

velo

LGTM, feel free to hit the button

velo · 2018-07-22T21:34:40Z

core/src/main/java/feign/FeignException.java

@@ -43,7 +43,7 @@ public int status() {

  static FeignException errorReading(Request request, Response ignored, IOException cause) {
    return new FeignException(
-        format("%s reading %s %s", cause.getMessage(), request.method(), request.url()),
+        format("%s reading %s %s", cause.getMessage(), request.httpMethod().name(), request.url()),


format should be able to deal with Enums just fine. no need to call name()

There is no need to have an explicit call to `Enum.name`. Removed. Style Compliance.

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 approved these changes Jul 19, 2018

View reviewed changes

Refactored Request Method Attribute on Requests

5993c5e

* Added `HttpMethod` enum that represents the supported HTTP methods replacing String handling. * Deprecated `Request#method()` in favor of `Request#httpMethod()`

kdavisk6 force-pushed the 719-retry-on-get-only branch from 47e071f to 5993c5e Compare July 20, 2018 16:25

kdavisk6 added the waiting for feedback Issues waiting for a response from either to the author or other maintainers label Jul 20, 2018

kdavisk6 mentioned this pull request Jul 20, 2018

Access to request, as well as response headers when response status != 2XX #396

Closed

Adding original Reqeust body to the Response

e918aca

velo approved these changes Jul 23, 2018

View reviewed changes

Removing unnecessary call to HttpMethods.name

bf3df0f

There is no need to have an explicit call to `Enum.name`. Removed. Style Compliance.

kdavisk6 merged commit 9875a16 into OpenFeign:master Jul 24, 2018

kdavisk6 deleted the 719-retry-on-get-only branch July 24, 2018 00:53

kdavisk6 removed the waiting for feedback Issues waiting for a response from either to the author or other maintainers label Aug 5, 2018

kdavisk6 added this to the 10.0 milestone Aug 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Method to Retryable Exception for evaluation #744

Adding Method to Retryable Exception for evaluation #744

kdavisk6 commented Jul 19, 2018

velo left a comment

velo Jul 19, 2018

kdavisk6 Jul 20, 2018 •

edited

Loading

velo Jul 19, 2018

kdavisk6 Jul 20, 2018

velo Jul 19, 2018

kdavisk6 Jul 20, 2018

velo Jul 19, 2018

kdavisk6 Jul 20, 2018

velo Jul 21, 2018

kdavisk6 Jul 21, 2018

velo Jul 19, 2018

velo Jul 19, 2018

kdavisk6 Jul 20, 2018

velo Jul 19, 2018

kdavisk6 Jul 20, 2018

kdavisk6 commented Jul 20, 2018

velo left a comment

velo Jul 22, 2018

		@@ -54,7 +58,7 @@ public static Request create(String method,

		/* Method to invoke on the server. */
		public String method() {

Adding Method to Retryable Exception for evaluation #744

Adding Method to Retryable Exception for evaluation #744

Conversation

kdavisk6 commented Jul 19, 2018

velo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdavisk6 Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdavisk6 commented Jul 20, 2018

velo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdavisk6 Jul 20, 2018 •

edited

Loading