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

spring-cloud-reactivefeign-core module added #11

Open
wants to merge 21 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@kptfh
Copy link

kptfh commented Mar 12, 2018

Fixes #4

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 12, 2018

Codecov Report

Merging #11 into master will increase coverage by 2.96%.
The diff coverage is 85.09%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #11      +/-   ##
============================================
+ Coverage     76.07%   79.04%   +2.96%     
- Complexity      283      448     +165     
============================================
  Files            36       61      +25     
  Lines          1183     1756     +573     
  Branches        183      220      +37     
============================================
+ Hits            900     1388     +488     
- Misses          206      262      +56     
- Partials         77      106      +29
Impacted Files Coverage Δ Complexity Δ
.../openfeign/reactive/client/ReactiveHttpClient.java 100% <100%> (ø) 4 <4> (?)
...openfeign/reactive/client/ReactiveHttpRequest.java 100% <100%> (ø) 5 <5> (?)
...framework/cloud/openfeign/reactive/utils/Pair.java 100% <100%> (ø) 1 <1> (?)
...active/client/StatusHandlerReactiveHttpClient.java 100% <100%> (ø) 5 <5> (?)
...e/client/statushandler/CompositeStatusHandler.java 36.36% <36.36%> (ø) 2 <2> (?)
...reactive/client/InterceptorReactiveHttpClient.java 50% <50%> (ø) 0 <0> (?)
...ork/cloud/openfeign/reactive/ReactiveRetryers.java 58.33% <58.33%> (ø) 3 <3> (?)
.../cloud/openfeign/reactive/ReactiveRetryPolicy.java 63.63% <63.63%> (ø) 3 <3> (?)
...cloud/openfeign/reactive/DefaultMethodHandler.java 64.7% <64.7%> (ø) 3 <3> (?)
...work/cloud/openfeign/reactive/utils/HttpUtils.java 68.42% <68.42%> (ø) 2 <2> (?)
... and 45 more
@spencergibb
Copy link
Member

spencergibb left a comment

Right off the bat, a couple of things.

Apply formatting from https://github.com/spring-cloud/spring-cloud-netflix/tree/master/eclipse

The package will need to change. feign is managed by openfeign. This would need to be org.springframework.cloud.openfeign.reactive

@kptfh

This comment has been minimized.

Copy link
Author

kptfh commented Mar 14, 2018

@spencergibb A bit confused with "Some checks were not successful" as I just renamed package and reformatted code

@ryanjbaxter

This comment has been minimized.

Copy link
Contributor

ryanjbaxter commented Mar 14, 2018

@kptfh its just because code coverage targets were not hit. We are not sticklers about it but if you want to try an increase the overage, that is always welcome.

@kptfh

This comment has been minimized.

Copy link
Author

kptfh commented Mar 14, 2018

@ryanjbaxter Plan to add some Unit tests but it will take some time.

@kptfh

This comment has been minimized.

Copy link
Author

kptfh commented Mar 19, 2018

@spencergibb @ryanjbaxter Renamed package, reformatted code, added tests.

@kptfh kptfh force-pushed the kptfh:master branch 4 times, most recently from 7fcf777 to 40cdf4e Mar 20, 2018

@kptfh kptfh force-pushed the kptfh:master branch from 7ad6a8f to 5ee630e Mar 20, 2018

import java.util.concurrent.TimeUnit;
import java.util.function.Function;

import feign.*;

This comment has been minimized.

@ryanjbaxter

ryanjbaxter Mar 20, 2018

Contributor

We dont use * imports

This comment has been minimized.

@kptfh

kptfh Mar 21, 2018

Author

fixed
Do you have formatter settings for Intellij Idea?

This comment has been minimized.

@kptfh

kptfh Mar 21, 2018

Author

@vasilievip doesn't work as expected. Actually this code is the result of applying this formmater with those settings.

&& ((ReactiveOptions) options).isTryUseCompression();

ReactorClientHttpConnector connector = new ReactorClientHttpConnector(
opts -> opts

This comment has been minimized.

@ryanjbaxter

ryanjbaxter Mar 20, 2018

Contributor

The spacing here makes this code hard to read

This comment has been minimized.

@ryanjbaxter

ryanjbaxter Mar 20, 2018

Contributor

Are these timeouts configurable?

This comment has been minimized.

@kptfh

kptfh Mar 21, 2018

Author

yes, you can specify them while building Feign client


public Builder<T> retryWhen(
Function<Flux<Throwable>, Publisher<Throwable>> retryFunction) {
this.retryFunction = retryFunction;

This comment has been minimized.

@ryanjbaxter

ryanjbaxter Mar 20, 2018

Contributor

How is the retry configured?

This comment has been minimized.

@kptfh

kptfh Mar 21, 2018

Author

The most generic way to configure retry for Mono or Flux is to provide retry function to retryWhen() method. There is ReactiveRetryers class with predefined retry functions.
This module doesn't have any spring configurations.

This comment has been minimized.

@kptfh

kptfh Mar 21, 2018

Author

This module is the most generic code that may be used without any cloud specific code and spring configurations. In cloud related module retries will be configured via Ribbon. Also spring configuration module is planned to be added later.

This comment has been minimized.

@ryanjbaxter

ryanjbaxter Mar 22, 2018

Contributor

So we can continue to use client.ribbon.MaxAutoRetries, client.ribbon.MaxAutoRetriesNextServer, and client.ribbon.OkToRetryOnAllOperations?

This comment has been minimized.

@kptfh

kptfh Mar 22, 2018

Author

Yes. After some work on configuration will be done. If you check RibbonReactiveClient you will see that it is based on com.netflix.loadbalancer.reactive.LoadBalancerCommand and it's configured via regular com.netflix.client.RetryHandler.

This comment has been minimized.

@kdavisk6

kdavisk6 Jul 15, 2018

Coming in a little late here, but why not contribute this Feign core? @kptfh you've lifted almost the entire class from feign-core?

.retrieve()
.onStatus(httpStatus -> decode404 && httpStatus == NOT_FOUND,
clientResponse -> null)
.onStatus(HttpStatus::isError, clientResponse -> clientResponse

This comment has been minimized.

@ryanjbaxter

ryanjbaxter Mar 20, 2018

Contributor

wow, can we break this code up a bit so it is easier to understand?

This comment has been minimized.

@kptfh

kptfh Mar 21, 2018

Author

fixed

skarpenko
@ryanjbaxter

This comment has been minimized.

Copy link
Contributor

ryanjbaxter commented Mar 22, 2018

We need to add some documentation. Also I think you need to add the module to the starter pom.

skarpenko added some commits Mar 22, 2018

@riba2101

This comment has been minimized.

Copy link

riba2101 commented May 6, 2018

Hey guys,

if you were building/testing something here's a workaround for the time being:

public class ReactiveDecoder implements Decoder {

    private final Decoder delegate;

    public ReactiveDecoder(Decoder delegate) {
        Objects.requireNonNull(delegate, "Decoder must not be null. ");
        this.delegate = delegate;
    }

  @Override
    public Object decode(Response response, Type type) throws IOException {
        boolean mono = isMono(type);
        boolean flux = isFlux(type);

        if (!flux && !mono) {
            return delegate.decode(response, type);
        }

        if (response.status() == 404) {
            if (mono) {
                return Mono.empty();
            }

            return Flux.empty();
        }

        if (mono) {
            Type   enclosedType = Util.resolveLastTypeParameter(type, Mono.class);
            Object decoded      = delegate.decode(response, enclosedType);
            if (decoded == null) {
                return Mono.empty();
            }

            return Mono.just(decoded);
        }

        Type   enclosedType = Util.resolveLastTypeParameter(type, Flux.class);
        Object decoded      = delegate.decode(response, enclosedType);
        if (decoded == null) {
            return Flux.empty();
        }

        return Flux.just(decoded);
    }

    private boolean isFlux(Type type) {
        if (!(type instanceof ParameterizedType)) {
            return false;
        }
        ParameterizedType parameterizedType = (ParameterizedType) type;
        return parameterizedType.getRawType().equals(Flux.class);
    }

    private boolean isMono(Type type) {
        if (!(type instanceof ParameterizedType)) {
            return false;
        }
        ParameterizedType parameterizedType = (ParameterizedType) type;
        return parameterizedType.getRawType().equals(Mono.class);
    }

}

and then substitute the default bean for this one:

    @Autowired
    private ObjectFactory<HttpMessageConverters> messageConverters;

    @Bean
    public Decoder feignDecoder() {
        return new ReactiveDecoder(new OptionalDecoder(new ResponseEntityDecoder(new SpringDecoder(this.messageConverters))));
    }

this worked for me.

Hope it helps someone

@ryanjbaxter

This comment has been minimized.

Copy link
Contributor

ryanjbaxter commented May 10, 2018

@kptfh do you plan on picking this back up?

@kptfh

This comment has been minimized.

Copy link
Author

kptfh commented May 18, 2018

@ryanjbaxter Sorry, but your questions is not clear to me. Do you mean to take a look at @riba2101's comment?

@ryanjbaxter

This comment has been minimized.

Copy link
Contributor

ryanjbaxter commented May 18, 2018

@kptfh no i mean completing the PR

@kptfh

This comment has been minimized.

Copy link
Author

kptfh commented May 18, 2018

@ryanjbaxter Sorry, treated your inactivity as you've lost any interest to this PR. Seemed that I fixed all your comments and had no feedback. So I made some bunch of changes in original repository and will need to redo all this stuff with renaming of packages again. Actually I still have a strong intention to complete this work but will not be able to do it without your help and good communication.

@ryanjbaxter

This comment has been minimized.

Copy link
Contributor

ryanjbaxter commented May 18, 2018

I was waiting for you to add documentation as I asked for above.

@kptfh

This comment has been minimized.

Copy link
Author

kptfh commented May 19, 2018

Literally, you said:

We need to add some documentation. Also I think you need to add the module to the starter pom

So I added module to the starter pom but didn't expect that you mean me to add doc.
Finally it's not clear what kind of doc to add: java doc(which classes?), how-to doc?

@ryanjbaxter

This comment has been minimized.

Copy link
Contributor

ryanjbaxter commented May 19, 2018

skarpenko added some commits May 24, 2018

skarpenko
skarpenko

@kptfh kptfh force-pushed the kptfh:master branch from 19896f2 to 6967ed2 May 24, 2018

@kptfh

This comment has been minimized.

Copy link
Author

kptfh commented May 24, 2018

Hi @ryanjbaxter. Added adoc to spring-cloud-reactivefeign-core. Want to remind that this module does't support Ribbon or Hystrix. This support will be added in the next "spring-cloud-reactivefeign-cloud" module

@kptfh kptfh force-pushed the kptfh:master branch from 43190fd to fd212f6 Jun 6, 2018

@kptfh

This comment has been minimized.

Copy link
Author

kptfh commented Jun 12, 2018

@ryanjbaxter Does it make sense to add spring-cloud-reactivefeign-cloud module (cloud specific logic - ribbon/hystrix support ) to this pull request to review it all together?

@vasilievip

This comment has been minimized.

Copy link

vasilievip commented Jul 7, 2018

@ryanjbaxter relase is out, can you merge changes or there is additional work required?

@ryanjbaxter

This comment has been minimized.

Copy link
Contributor

ryanjbaxter commented Jul 9, 2018

Hopefully we can get this merged this week. We might be doing another Finchley service release earlier than planned do to an issue that popped up in another project. So I might wait until after that.

skarpenko
* Handles default methods by directly invoking the default method code on the interface.
* The bindTo method must be called on the result before invoke is called.
*/
final class DefaultMethodHandler implements MethodHandler {

This comment has been minimized.

@kdavisk6

kdavisk6 Jul 15, 2018

@spencergibb @ryanjbaxter This class is a direct copy from the feign core. I don't think you'll want to accept this verbatim. This class is not exposed in the public feign api.

@kdavisk6

This comment has been minimized.

Copy link

kdavisk6 commented Jul 16, 2018

I am coming into this very late, and I don't want to derail this, but coming from feign, this PR contains more feign code than Spring. In fact, with a little more refactoring, this could be a PR into feign directly. I'm confused as to why @kptfh you did not consider submitting this to feign?

@spencergibb

This comment has been minimized.

Copy link
Member

spencergibb commented Jul 16, 2018

I hadn't realized that with a minor refactor it could go in an open feign module. I think that might be a good idea. Reactor could certainly be used outside of spring.

@kdavisk6

This comment has been minimized.

Copy link

kdavisk6 commented Jul 16, 2018

I don't see any reason why we could not create a feign-reactive-reactor sub module with the changes included in spring-cloud-reactivefeign-core module. The refactoring would be around the Retry, as the current Retryer interface in feign would not work in this case, but I'm sure we can make it work.

This feature has been widely requested within feign as mentioned in issues OpenFeign/feign#622 and OpenFeign/feign#644 and OpenFeign/feign#361. feign is now baselined on JDK 8, so I don't see anything really in the way here.

I'm more than willing to help out where I can with this. Reactor is a good a place to start as any. If this goes well, we can use this as the blueprint to add support for RxJava for feigns Android users.

@kptfh

This comment has been minimized.

Copy link
Author

kptfh commented Jul 16, 2018

@kdavisk6 This version of reactive feign depends on Spring's WebClient as reactive http transport.
Also don't see a lot of benefits to use reactive approach on Android as it suits more highly loaded server side (saves memory and CPU on reducing number of threads)

@kdavisk6

This comment has been minimized.

Copy link

kdavisk6 commented Jul 16, 2018

Looking over the PR again, I can see that the WebReactiveHttpClient is the only component that relies on Spring. The rest of the PR is pure Feign.

The way this PR is constructed currently, it copies a few key core objects from feign-core verbatim into spring-cloud. This could cause issues in the future. For example, #34 requests that the spring-cloud update to OpenFeign 10.x, which will break this integration. Also, @spencergibb has mentioned in the #4 that he would prefer that nothing of feign make it into spring-boot. This PR will do just that, it will now have it's own copy of feign will be decoupled from feign without a fork.

One way to ensure that both projects can benefit is to take the Feign related items such as ReactiveHttpClient, ReactiveFeign, ReactiveEncoder, ReactiveDecoder, etc... and add them to OpenFeign under feign-reactive and keep the WebReactiveHttpClient here with Spring Cloud. This way, both projects get the benefit. feign will have the reactive framework and Spring is still able to take advantage of it without the duplication. Thoughts?

@kptfh

This comment has been minimized.

Copy link
Author

kptfh commented Jul 16, 2018

Sounds reasonable. Just a few comments from my side:

  1. There are no ReactiveEncoder and ReactiveDecoder as WebClient takes care of this
  2. The only value in this PR except of WebReactiveHttpClient are reactive logger and reactive retryers. All other code is related to WebReactiveHttpClient
  3. It will take decent time to introduce alternative ReactiveHttpClient implementation. Not sure have enough time to invest in this.

Is it acceptable to commit in open feign just a draft of framework without implementation and implementation based on WebClient here?

@kdavisk6

This comment has been minimized.

Copy link

kdavisk6 commented Jul 16, 2018

@kptfh We can certainly create a repository under OpenFeign similar to what was done for Vert.x for this work. I'm volunteering my time to help you make this happen. To help coordinate, can we move that discussion over to OpenFeign/feign#644?

@kptfh

This comment has been minimized.

Copy link
Author

kptfh commented Jul 17, 2018

@spencergibb what do you think about this?

  1. Leave PR here as is or
  2. split into open feign part (reactive framework ReactiveHttpClient without implementation) and Spring part (based on WebClient)?
@ryanjbaxter

This comment has been minimized.

Copy link
Contributor

ryanjbaxter commented Jul 17, 2018

I think it makes the most sense to keep as much as we can in OpenFeign.

@spencergibb

This comment has been minimized.

Copy link
Member

spencergibb commented Jul 17, 2018

👍

@kdavisk6

This comment has been minimized.

Copy link

kdavisk6 commented Jul 17, 2018

I've created a repository for you https://github.com/OpenFeign/feign-reactive. Please feel free to reach out to me if you need anything else.

skarpenko and others added some commits Jul 23, 2018

Merge remote-tracking branch 'origin/master'
# Conflicts:
#	docs/src/main/asciidoc/spring-cloud-openfeign.adoc
#	spring-cloud-reactivefeign-core/src/main/java/org/springframework/cloud/openfeign/reactive/ReactiveFeign.java
#	spring-cloud-reactivefeign-core/src/main/java/org/springframework/cloud/openfeign/reactive/client/statushandler/SimpleStatusHandler.java
#	spring-cloud-reactivefeign-core/src/test/java/org/springframework/cloud/openfeign/reactive/RequestInterceptorTest.java
#	spring-cloud-reactivefeign-core/src/test/java/org/springframework/cloud/openfeign/reactive/RetryingTest.java
#	spring-cloud-reactivefeign-core/src/test/java/org/springframework/cloud/openfeign/reactive/StatusHandlerTest.java
@kptfh

This comment has been minimized.

Copy link
Author

kptfh commented Aug 22, 2018

@spencergibb @ryanjbaxter Can you comment on this issue OpenFeign/feign-reactive#15 ?

@kptfh

This comment has been minimized.

Copy link
Author

kptfh commented Sep 11, 2018

@ryanjbaxter @spencergibb stack into dead end with openfeign repo. Don't see any benefits to keep code there. No one is reviewing the code except one guy (@kdavisk6) that don't understand how reactive approach should work. It will last for years and from my side looks like sabotage or time killing.
Please, take a look at this issue OpenFeign/feign-reactive#15 and pull request OpenFeign/feign-reactive#18

@Aloren

This comment has been minimized.

Copy link
Contributor

Aloren commented Sep 28, 2018

hello @spencergibb, could you please advise what should be done to proceed communication on this pr? obviously, there is no common vision of what should or should not be done with reactive-feign right now. maybe we could find some temporary solutions for the not agreed issues arisen during review?

@ryanjbaxter

This comment has been minimized.

Copy link
Contributor

ryanjbaxter commented Oct 1, 2018

@Aloren we do not want to duplicate code from openfeign in this repo. I would rather keep this downstream. From OpenFeign/feign-reactive#15 it sounds like the downstream code was going to be moved to https://github.com/kptfh/feign-reactive. Maybe the best approach then is to have a separate corresponding repo for an reactive spring cloud openfeign module that depends on that code. I could talk to the team and see if they would mind that being a community driven project in our incubator if the community is interested in that. Once the code in https://github.com/kptfh/feign-reactive can be moved into OpenFeign then we consider moving that incubator module into this project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.