Consider releasing artifact

[kptfh]

There are tons of devs waiting for working ReactorFeign. Especially they are interested in Spring's implementation (witht Ribbon/Hystrix support).
@kdavisk6 Let's release this artifact so I can proceed with Spring's WebClient implementation.

[velo]

@kdavisk6 and I talked about this.

I think we should release this as io.github.openfeign.incubation until we get a solid api.

What do you think?

Can I begin the release right now?

[kptfh]

@spencergibb @ryanjbaxter are you OK with it? Will you approve the pull request in Spring Cloud with io.github.openfeign.incubation dependency ?

[kptfh]

@velo @kdavisk6 How do you estimate the time you need to get solid API?

[velo]

I think when we have all the API tickets solved, it will be a solid and stable api good for a release
https://github.com/OpenFeign/feign-reactive/issues?q=is%3Aopen+is%3Aissue+label%3AAPI

They may require work to be done on feign-core as well

[kdavisk6]

Apologies for the delay in responding, I was away with the family. I am just about finished a few bug fixes for core. I can help starting later this week.

[ryanjbaxter]

As long as the API in the Spring Cloud project is stable we wouldnt mind using something that is incubating but if that Spring Cloud API could change as well than we will have to take a different approach.

[kptfh]

@kdavisk6 @velo Ok, guys. Let's release it somehow. Don't want to stop progress. Probably for someone it may be determinative that java has reactive feign client while choosing language for project.
JFMI, how do you estimate time it may take to solve all API issue?

[kdavisk6]

I've started looking into the API issues and I believe that most of these API issues can be solved if we change the approach being taken. Most of this library is a refactor of core feign concepts using functional style and paradigms. The reactive areas are limited to executing the Request, which we can simplify by wrapping the existing synchronous implementation in a Reactive wrapper.

I've started working on that in my fork and you can see what I have here:

https://github.com/kdavisk6/feign-reactive/tree/reactive-core/feign-reactive-core/src/main/java/feign

From my perspective, this should be sufficient for us to get an initial release out. Since this approach extends from feign directly, it will allow this library to support everything regular feign does, including logging, encoding, decoding, OkHttp, ApacheHTTP, Jackson, GSON, etc... solving all of our API issues.

With regards to Spring Cloud, all that should be necessary is a new Targeter, as my approach is extremely close to HystrixFeign. Please take a look at my fork and let me know what you all think. @spencergibb and @ryanjbaxter I would appreciate if you could check my assumptions and provide us with your perspective on this problem.

@velo and @kptfh I would appreciate if you could look at my fork and let me know your thoughts.

[kptfh]

which we can simplify by wrapping the existing synchronous implementation in a Reactive wrapper.

@kdavisk6 Reactive programming is not just interface it's also means reactive implementation behind the scene. Wrapping sync call with reactive interface will not make them reactive. You need all the chain to be reactive and at the end of this chain is reactive http client should be. But in your approach whole chain is not reactive and it kills any gains that reactivity can give.
BTW How are you going to process Flux with this approach?

[kptfh]

The main benefit reactive approach gives is low threads consumption. But with your approach threads usage remains high (one per call)

[kdavisk6]

I agree with you. A fully reactive Feign is the goal of this project.

My proposal is a trade off. If accepted, we would be able to release something in the near term, clearing any blocks on other downstream projects. We can then take the time we need to refactor each API component, updating core if necessary, taking an iterative approach. The alternative is to wait until we have refactored every API and closed every issue.

I'm trying to find a balance here, between moving things forward without moving too quickly.

[kptfh]

dead end

[kdavisk6]

I'm sorry you feel that way. The personal jab is unnecessary and immature, however. I understand fully what a reactive approach entails; however, feign historically has been synchronous and you can see from OpenFeign/feign#644 and OpenFeign/feign#361 that making feign reactive is not a simple nor straightforward undertaking. Simply adopting one of the current reactive-streams implementations is not enough.

Feign strives to keep dependencies light. Taken directly from HACKING

Code design is opinionated including below:

• Classes and methods default to package, not public visibility.
• Changing certain implementation classes may be unsupported.
• 3rd-party dependencies, and gnarly apis like java.beans are avoided.

Staying true to these principles means that we must consider the impact each new dependency has to those using Feign. It's not fair for us to impose one dependency/library over another. Doing so limits the flexibility and reach of Feign. We also must also consider maintenance going forward, again from HACKING

If you look carefully, you'll notice Feign integrations are often less than 1000 lines of code including tests. Some features are rejected for inclusion solely due to the amount of maintenance. For example, adding some features might imply tying up maintainers for several days or weeks and resulting in a large percentage increase in the size of feign.

This is another principle that we do our best to follow. Who is going to be responsible for updating this library as reactor and reactivex continue to evolve? How does that impact those using this library? All of this speaks to why I'm asking us to consider and come to an agreement on how this should be built and maintained.

I haven't even begun to talk about what the overall impact this has on the core concepts within Feign and how a decision like this would effect them. Simply put, as OpenFeign stands today, building a fully reactive approach a serious undertaking. @adriancole's thoughts sum this up pretty well:

What do you mean by "which is what feign isn't designed for"?
Feign's is not designed for asynchronous invocation or zero-copy i/o.

For example, requests are buffered up-front, and all i/o operations are blocking. Core concepts such as interceptors and exception handlers were not designed for asynchronous invocation.

The Client interface (required by all http backends) is a synchronous api. Internal dispatch assumes synchronous apis, as well many try/catch blocks associated with it. RetryHandler assumes synchronous invocation

I could go on, but there are a lot of internals that are simpler because synchronous is assumed. When someone changes the invocation model, they must break api or certain things won't work. Also, depending on what's desired from nio, the contents of requests and responses may also need to be changed.

Hope this helps.

What we have in this repository now, is a highly opinionated approach aimed at solving a single use case involving reactor use. To do this right, we need to identify which core concepts need refactoring and then make the appropriate changes so we can support both synchronous and non-blocking "reactive" models. I truly believe that the results will benefit everyone involved and I'm not ready to give up on this.

[velo]

Hi @kptfh, seems we can not agree on implementation details on feign-reactive

Pretty much, @kdavisk6 and I wanna see more interface to break it down and reuse feign-core and you wanna see it as is to get Spring Cloud work done.

After today drama, I don't think we are going to be able to re-conciliate this easily.

What I propose is to transfer ownership of OpenFeign/feign-reactive to you and so you can control it fully.

Transfer ownership would move the feign-reactive from OpenFeign to kptfh
anyone hitting https://github.com/OpenFeign/feign-reactive would be redirected to https://github.com/kptfh/feign-reactive

Lemme know if that you are interested on it.

[kptfh]

Hi @velo, thanks for being community member.
From the very beginning it was clear that we have totally different vision and targets regarding this repo.
From my side I just want to give community first truly reactive implementation of Feign (Spring WebClient based) and give framework and interfaces that may allow someone to implement it on other reactive http client (Jetty/Netty). And it's was not obvious why community should wait for years if there is already fully functioning version of Reactive Feign.
From your side it was a place to experiment on

• generic interfaces (CompletableFuture, Rx, ...)
• how to mutate existing sync Feign into reactive and reuse most of old Feign

You can recall that I've offered you to rename repo into "reactor" instead of "reactive" and narrow it scope.
So it would be the best solution it we finally split it into 2 different repo:
"reactor" - already implemented reactor Feign that community should get ASAP and
"reactive" - field for experiments and small steps that may last for years
Regarding redirecting to https://github.com/kptfh/feign-reactive it doesn't matter where it will be hosted. Do it as you wish.

[kptfh]

@velo @kdavisk6 Guys it's your turn. Better make some decision and move forward.

[prafsoni]

@kdavisk6 @kptfh @velo
I don't know if this came up in for discussions. But, Why don't you guys take the same approach as Spring team? They solved the problem of supporting multiple reactive libs, by making their interfaces return and accept reactive-streamsPulblisher<T> which all reactive libs implement. For the internal implementation pick any popular and supported lib of choice RxJava or Reactor.

[kdavisk6]

@prafsoni

On the surface, that is what we are looking to do. The complexity lies in determining which interfaces should be updated and how the internal proxy should work, while trying to maintain some compatibility to the existing extensions. If you take a look at OpenFeign/feign#361 and OpenFeign/feign#644, you see some of the background on this issue. In it, there are two proposals:

1. "Retro"-fit reactive into it. This is what I tried to do to get us off the ground in WIP: Reactive Wrapper for Synchronous Method Handler #18 and in Reactive Wrapper Support feign#795. These add support for reactive Publisher return types, but are not fully reactive, as the execution of the method is still synchronous/blocking.
2. Fully non-blocking support. This can be achieved but as called out, will require major API changes.

Which brings us to where we are today. There is a difference of opinion on if we should take and release a version of with that adheres to the first option, via "retro-fitting", release a version with a fully reactive Client only, or if we should rebuild the entire framework from the ground up.

@prafsoni, what are your thoughts? What would you like to see come out of this effort?

[vasilievip]

Looking at applicability page here (which makes sense to me): https://docs.spring.io/spring/docs/current/spring-framework-reference/web-reactive.html#webflux-framework-choice

A simple way to evaluate an application is to check its dependencies. If you have blocking persistence APIs (JPA, JDBC) or networking APIs to use, Spring MVC is the best choice for common architectures at, least. It is technically feasible with both Reactor and RxJava to perform blocking calls on a separate thread but you would not be making the most of a non-blocking web stack.

It tells me that if I'm trying to mix blocking with non-blocking stacks into one jar, I'm still getting blocking code with reactivity overhead. So, option 2 sounds most reasonable long term. Blocking feign is already stable and works just fine, but it is blocking by design. To make most of non blocking stack - all pieces (logging, parsing etc) needs to be redesigned. I do not see reason to me to plug into project reactive wrapper for non reactive feign.
There is several reactive implementations, but looking at sample implementation already provided by @kptfh - https://github.com/kptfh/feign-reactive#rx2-usage, there is no issues to support them.