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

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

Merged
merged 3 commits into from Feb 12, 2023

Conversation

nlsbchmnn
Copy link
Contributor

removed deprecated usage of Schedulers.elastic() and using Schedulers.boundedElastic() instead

Fixes #1711

removed deprecated usage of <code>Schedulers.elastic()</code> and using <code>Schedulers.boundedElastic()</code> instead

Fixes OpenFeign#1711
@@ -30,7 +30,7 @@ public static Builder builder() {

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

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...

Created a second builder method which allows to configure the scheduler as a parameter. The default builder method now uses the non deprecated <code>Schedulers.boundedElastic()</code>

Fixes OpenFeign#1711
@velo velo merged commit aba4b75 into OpenFeign:master Feb 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feign-reactive-wrappers:11.9.1 not compatible with reactor-core:3.5.0
2 participants