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

updates reactor-core dependency for the feign-reactive-wrappers #1930

Merged
merged 3 commits into from
Feb 12, 2023
Merged

updates reactor-core dependency for the feign-reactive-wrappers #1930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f6a8da0
updates reactor-core dependency for the feign-reactive-wrappers
nlsbchmnn Feb 8, 2023
86ef9e5
Merge branch 'master' into update_reactor_core_dep
velo Feb 10, 2023
e1c9145
moved scheduler initialization to constructor
nlsbchmnn Feb 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion reactive/pom.xml
View file
Open in desktop
Expand Up @@ -28,7 +28,7 @@

<properties>
<main.basedir>${project.basedir}/..</main.basedir>
<reactor.version>3.4.24</reactor.version>
<reactor.version>3.5.2</reactor.version>
<reactive.streams.version>1.0.4</reactive.streams.version>
<reactivex.version>2.2.21</reactivex.version>
</properties>
Expand Down
17 changes: 10 additions & 7 deletions reactive/src/main/java/feign/reactive/ReactorFeign.java
View file
Open in desktop
Expand Up @@ -25,12 +25,20 @@
public class ReactorFeign extends ReactiveFeign {

public static Builder builder() {
return new Builder();
return new Builder(Schedulers.boundedElastic());
}

public static Builder builder(Scheduler scheduler) {
return new Builder(scheduler);
}

public static class Builder extends ReactiveFeign.Builder {

private Scheduler scheduler = Schedulers.elastic();
Copy link
Member

Choose a reason for hiding this comment

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

Problem is this breaks binary compatibility.... is so annoying when projects break compatibility on minor releases

Copy link
Member

Choose a reason for hiding this comment

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

can we make Scheduler scheduler a parameter, and if never set, we default to Schedulers.boundedElastic()?

And then document how to use 3.4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for my understanding: Why is it breaking binary compatibility?

It appears to me that the scheduler is already a parameter, which you can set with public Builder scheduleOn(Scheduler scheduler) and Schedulers.elastic() is the current default.

Copy link
Member

@velo velo Feb 10, 2023 •
edited

Choose a reason for hiding this comment

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

elastic to boundedElastic breaks anyone using the old version of the library.

Tell you what, as a compromise, move the initialization to constructor and have one that allows user to pick how to create the scheduler and the other defaults to new code

  public static Builder builder() {
    return new Builder(Schedulers.boundedElastic());
  }

  public static Builder builder(Scheduler scheduler) {
    return new Builder(scheduler);
  }

  public static class Builder extends ReactiveFeign.Builder {

    private Scheduler scheduler;

.. constructor...

private final Scheduler scheduler;

Builder(Scheduler scheduler) {
this.scheduler = scheduler;
}

@Override
public Feign build() {
Expand All @@ -43,11 +51,6 @@ public Builder invocationHandlerFactory(InvocationHandlerFactory invocationHandl
throw new UnsupportedOperationException(
"Invocation Handler Factory overrides are not supported.");
}

public Builder scheduleOn(Scheduler scheduler) {
this.scheduler = scheduler;
return this;
}
}

private static class ReactorInvocationHandlerFactory implements InvocationHandlerFactory {
Expand Down
6 changes: 3 additions & 3 deletions reactive/src/test/java/feign/reactive/ReactiveInvocationHandlerTest.java
View file
Open in desktop
Expand Up @@ -56,7 +56,7 @@ public void setUp() throws NoSuchMethodException {
public void invokeOnSubscribeReactor() throws Throwable {
given(this.methodHandler.invoke(any())).willReturn("Result");
ReactorInvocationHandler handler = new ReactorInvocationHandler(this.target,
Collections.singletonMap(method, this.methodHandler), Schedulers.elastic());
Collections.singletonMap(method, this.methodHandler), Schedulers.boundedElastic());

Object result = handler.invoke(method, this.methodHandler, new Object[] {});
assertThat(result).isInstanceOf(Mono.class);
Expand All @@ -74,7 +74,7 @@ public void invokeOnSubscribeReactor() throws Throwable {
public void invokeOnSubscribeEmptyReactor() throws Throwable {
given(this.methodHandler.invoke(any())).willReturn(null);
ReactorInvocationHandler handler = new ReactorInvocationHandler(this.target,
Collections.singletonMap(method, this.methodHandler), Schedulers.elastic());
Collections.singletonMap(method, this.methodHandler), Schedulers.boundedElastic());

Object result = handler.invoke(method, this.methodHandler, new Object[] {});
assertThat(result).isInstanceOf(Mono.class);
Expand All @@ -91,7 +91,7 @@ public void invokeOnSubscribeEmptyReactor() throws Throwable {
public void invokeFailureReactor() throws Throwable {
given(this.methodHandler.invoke(any())).willThrow(new IOException("Could Not Decode"));
ReactorInvocationHandler handler = new ReactorInvocationHandler(this.target,
Collections.singletonMap(this.method, this.methodHandler), Schedulers.elastic());
Collections.singletonMap(this.method, this.methodHandler), Schedulers.boundedElastic());

Object result = handler.invoke(this.method, this.methodHandler, new Object[] {});
assertThat(result).isInstanceOf(Mono.class);
Expand Down