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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

meifans
Copy link
Contributor

@meifans meifans commented May 9, 2019

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
Copy link
Member

kdavisk6 commented May 9, 2019

@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
Copy link
Member

@kdavisk6 kdavisk6 left a comment

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?

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)
Copy link
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
Copy link
Member

@velo velo left a comment

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
Copy link
Member

velo commented May 16, 2019

😮

@meifans
Copy link
Contributor Author

meifans commented May 17, 2019

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