Skip to content

Adds support for per request timeout options. Fixes #562#961

Closed
meifans wants to merge 2 commits into
OpenFeign:masterfrom
meifans:feature-562
Closed

Adds support for per request timeout options. Fixes #562#961
meifans wants to merge 2 commits into
OpenFeign:masterfrom
meifans:feature-562

Conversation

@meifans

@meifans meifans commented May 9, 2019

Copy link
Copy Markdown
Contributor

Adds support for per request timeout options
Fixes #562

As @kdavisk6 said

interface MyClient {
   @RequestLine("GET /data")
   public MyResponse getMyData(Request.Options options);
}

In Spring Cloud

interface MyClient {
   @RequestMapping(path = "/data")
   public MyResponse getMyData(Request.Options options);
}

@kdavisk6

kdavisk6 commented May 9, 2019

Copy link
Copy Markdown
Member

@meifans

Thank you for this PR. Before we continue, please correct the formatting changes included in the PR by running mvn clean install and pushing the updated files.

kdavisk6
kdavisk6 requested changes May 9, 2019
View reviewed changes

@kdavisk6 kdavisk6 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@meifans

This looks very good. In addition to my other comments, can you add a Unit Test?

Comment thread core/src/main/java/feign/SynchronousMethodHandler.java Outdated
if (argv == null || argv.length == 0) {
return this.options;
}
return Stream.of(argv).filter(o -> o instanceof Options).findAny().map(o -> (Options) o)

@kdavisk6 kdavisk6 May 9, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe you need the findAny(). This can be simplified:

return (Options) Stream.of(argv)
         .filter(arg -> arg instanceof Options)
         .findFirst()
         .orElse(this.options);

@kdavisk6 kdavisk6 added enhancement For recommending new capabilities waiting for feedback Issues waiting for a response from either to the author or other maintainers labels May 9, 2019
velo
velo requested changes May 12, 2019
View reviewed changes

@velo velo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few concerns about this change....

First being not tests.

I think this will cause problems when combining body and options

@meifans
Adds support for per request timeout options. Fixes OpenFeign#562
ad4cf55 
  * Aadd Options Test
  * Ignore Options when set bodyIndex
@meifans meifans closed this May 16, 2019
@meifans meifans deleted the feature-562 branch May 16, 2019 07:50
@velo

velo commented May 16, 2019

Copy link
Copy Markdown
Member

😮

@meifans

meifans commented May 17, 2019

Copy link
Copy Markdown
Contributor Author

A new PR: #970

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@velo velo velo requested changes
@kdavisk6 kdavisk6 kdavisk6 requested changes

Assignees

No one assigned

Labels

enhancement For recommending new capabilities waiting for feedback Issues waiting for a response from either to the author or other maintainers

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Per request timeout options

3 participants

@meifans @kdavisk6 @velo