Support RxJava 3 #2501

ikhoon · 2020-02-17T14:23:48Z

Motivation:
Recently RxJava 3 has been released.
https://github.com/ReactiveX/RxJava/releases/tag/v3.0.0
Armeria supports RxJava2 integration with armeria-rxjava.
This PR migrates RequestContextAssembly from RxJava2 to RxJava3.
RxJava 3 being based on Java 8 also supports seamless conversions between CompletionStage and Single, Maybe, Observable.

Modifications:

Rename original armeria-rxjava to armeria-rxjava2
Make armeria-rxjava support RxJava 3
Use built-in converter methods for ObservableResponseConverterFunction
Change *Callable to *Supplier
3.x: Widen functional interface throws, replace Callable with Supplier ReactiveX/RxJava#6511
Remove assemblyContext.push() in RequestContextScalarCallable*.get()
- Don't need to push RequestContext for scalar type such as Maybe.just(T)
Delegate reset() method to RequestContextConnectableFlowable, RequestContextConnectableObservable
https://github.com/ReactiveX/RxJava/wiki/What's-different-in-3.0#connectable-source-reset
Migrate examples/context-propagation/rxjava to RxJava 3

Result:
You can now use RxJava 3 with Armeria.
Fixes: #2378

Motivation: Recently RxJava 3 has been released. https://github.com/ReactiveX/RxJava/releases/tag/v3.0.0 Armeria supports RxJava2 integration with `armeria-rxjava`. This PR migrates `RequestContextAssembly` from RxJava2 to RxJava3. RxJava 3 being based on Java 8 also supports seamless conversions between CompletionStage and Single, Maybe, Observable. Modifications: * Rename original `armeria-rxjava` to `armeria-rxjava2` * Make armeria-rxjava support RxJava 3 * Use built-in converter methods for ObservableResponseConverterFunction * Change `*Callable` to `*Supplier` ReactiveX/RxJava#6511 * Don't migrate `RequestContextScalarCallableCompletable` and `RequestContextCallableCompletable` There is no subclasses of `Completable` that implement `Supplier` * Delegate `reset()` in `ConnectableFlowable` https://github.com/ReactiveX/RxJava/wiki/What's-different-in-3.0#connectable-source-reset Result: You can now use RxJava 3 with Armeria. Fixes: line#2378

ikhoon · 2020-02-17T14:28:37Z

@anuraaga @kojilin might be interested in this PR. 😉

ikhoon · 2020-02-18T02:45:16Z

rxjava/src/main/java/com/linecorp/armeria/rxjava/RequestContextScalarSupplierMaybe.java

+    public T get() {
+        try (SafeCloseable ignored = assemblyContext.push()) {
+            return ((ScalarSupplier<T>) source).get();
+        }
+    }


The value of ScalarSupplier, which was ScalarCallable in RxJava 2, was captured when calling Maybe.just(T).
I wonder do we need to push RequestContext here? If not, I think we could remove all RequestContextScalarSupplier*. 🧐

I think we don't have to push the context here. 😄

On second thought, I think there a chance that a user specifies a class that extends Maybe and implements ScalarSupplier.
The specified class might need the context in get() method, so I think we need this.

Hmm... ScalarSupplier is the same as Supplier except it abandons throws Throwable. If a user triggers effects in get() method, it seems a wrong usage of ScalarSupplier. Because it should hold const and scalar value.
https://github.com/ReactiveX/RxJava/blob/3.x/src/main/java/io/reactivex/rxjava3/internal/fuseable/ScalarSupplier.java#L19
I think the user has to use Supplier for generating a value dynamically.

Ah, yes. That's a good point. 👍

codecov · 2020-02-18T04:25:27Z

Codecov Report

Merging #2501 into master will decrease coverage by 0.21%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #2501      +/-   ##
============================================
- Coverage     72.98%   72.76%   -0.22%     
- Complexity    10841    10930      +89     
============================================
  Files           947      972      +25     
  Lines         41864    42310     +446     
  Branches       5182     5234      +52     
============================================
+ Hits          30554    30788     +234     
- Misses         8659     8852     +193     
- Partials       2651     2670      +19

Impacted Files	Coverage Δ	Complexity Δ
...nal/server/tomcat/ConfigFileLoaderInitializer.java	`42.85% <0.00%> (-42.86%)`	`1.00% <0.00%> (ø%)`
...m/linecorp/armeria/common/util/AbstractOption.java	`61.19% <0.00%> (-38.81%)`	`11.00% <0.00%> (+10.00%)`
...m/linecorp/armeria/client/ClientFactoryOption.java	`70.31% <0.00%> (-29.69%)`	`7.00% <0.00%> (+3.00%)`
...java/com/linecorp/armeria/client/ClientOption.java	`77.35% <0.00%> (-22.65%)`	`11.00% <0.00%> (+7.00%)`
...meria/common/stream/RegularFixedStreamMessage.java	`81.63% <0.00%> (-6.13%)`	`13.00% <0.00%> (-1.00%)`
...ria/internal/common/logging/LoggingDecorators.java	`86.20% <0.00%> (-5.10%)`	`7.00% <0.00%> (ø%)`
...ecorp/armeria/internal/common/grpc/GrpcStatus.java	`68.04% <0.00%> (-2.93%)`	`33.00% <0.00%> (+1.00%)`
...corp/armeria/testing/junit4/server/ServerRule.java	`51.28% <0.00%> (-2.57%)`	`13.00% <0.00%> (-1.00%)`
...m/linecorp/armeria/internal/client/ClientUtil.java	`94.59% <0.00%> (-2.38%)`	`10.00% <0.00%> (+1.00%)`
...corp/armeria/common/NonWrappingRequestContext.java	`81.13% <0.00%> (-1.89%)`	`22.00% <0.00%> (ø%)`
... and 67 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96c5a99...c81cf78. Read the comment docs.

rxjava/src/main/java/com/linecorp/armeria/rxjava/RequestContextAssembly.java

rxjava2/src/main/java/com/linecorp/armeria/server/rxjava2/package-info.java

rxjava/src/main/java/com/linecorp/armeria/internal/server/rxjava/package-info.java

minwoox

👍

kojilin · 2020-02-26T23:29:02Z

rxjava/src/main/java/com/linecorp/armeria/common/rxjava/RequestContextSupplierFlowable.java

+    @SuppressWarnings("unchecked")
+    @Override
+    public T get() throws Throwable {
+        try (SafeCloseable ignored = assemblyContext.push()) {


As the comment above, do we need to remove here too? I still see some ScalarSupplier has context.

This implements Supplier<T>, not ScalarSupplier<T>. So I think this is fine. 😀

trustin · 2020-02-27T07:03:30Z

Thanks a lot, @ikhoon !

ikhoon · 2020-02-27T07:04:13Z

Thanks for reviewing! 🙏

Motivation: Recently RxJava 3 has been released. https://github.com/ReactiveX/RxJava/releases/tag/v3.0.0 Armeria supports RxJava2 integration with `armeria-rxjava`. This PR migrates `RequestContextAssembly` from RxJava2 to RxJava3. RxJava 3 being based on Java 8 also supports seamless conversions between CompletionStage and Single, Maybe, Observable. Modifications: * Rename original `armeria-rxjava` to `armeria-rxjava2` * Make `armeria-rxjava` support RxJava 3 * Use built-in converter methods for ObservableResponseConverterFunction * Change `*Callable` to `*Supplier` ReactiveX/RxJava#6511 * Remove `assemblyContext.push()` in `RequestContextScalarCallable*.get()` * Don't need to push `RequestContext` for scalar type such as `Maybe.just(T)` * Delegate `reset()` method to `RequestContextConnectableFlowable`, `RequestContextConnectableObservable` https://github.com/ReactiveX/RxJava/wiki/What's-different-in-3.0#connectable-source-reset * Migrate `examples/context-propagation/rxjava` to RxJava 3 Result: You can now use RxJava 3 with Armeria. Fixes: line#2378

ikhoon added breaking change dependencies labels Feb 17, 2020

ikhoon requested review from minwoox and trustin as code owners February 17, 2020 14:23

ikhoon requested a review from kojilin February 17, 2020 14:23

trustin added this to the 1.0.0 milestone Feb 17, 2020

ikhoon commented Feb 18, 2020

View reviewed changes

Fix wrong package name

26e5f85

trustin reviewed Feb 20, 2020

View reviewed changes

Address comments by @trustin / Clean up

234203e

minwoox approved these changes Feb 25, 2020

View reviewed changes

Remove context push in RequestContextScala*.call()

ff5f88f

ikhoon force-pushed the rxjava3 branch from 4d87666 to ff5f88f Compare February 25, 2020 13:56

Fix indent

c81cf78

kojilin reviewed Feb 26, 2020

View reviewed changes

trustin approved these changes Feb 27, 2020

View reviewed changes

trustin modified the milestones: 1.0.0, 0.98.4 Feb 27, 2020

trustin merged commit 519dbca into line:master Feb 27, 2020

ikhoon deleted the rxjava3 branch February 28, 2020 05:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support RxJava 3 #2501

Support RxJava 3 #2501

ikhoon commented Feb 17, 2020 •

edited

Loading

ikhoon commented Feb 17, 2020

ikhoon Feb 18, 2020 •

edited

Loading

minwoox Feb 25, 2020

minwoox Feb 25, 2020

ikhoon Feb 25, 2020 •

edited

Loading

minwoox Feb 26, 2020

codecov bot commented Feb 18, 2020 •

edited

Loading

minwoox left a comment

kojilin Feb 26, 2020 •

edited

Loading

ikhoon Feb 27, 2020

trustin commented Feb 27, 2020

ikhoon commented Feb 27, 2020

Support RxJava 3 #2501

Support RxJava 3 #2501

Conversation

ikhoon commented Feb 17, 2020 • edited Loading

ikhoon commented Feb 17, 2020

ikhoon Feb 18, 2020 • edited Loading

Choose a reason for hiding this comment

minwoox Feb 25, 2020

Choose a reason for hiding this comment

minwoox Feb 25, 2020

Choose a reason for hiding this comment

ikhoon Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

minwoox Feb 26, 2020

Choose a reason for hiding this comment

codecov bot commented Feb 18, 2020 • edited Loading

Codecov Report

minwoox left a comment

Choose a reason for hiding this comment

kojilin Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

ikhoon Feb 27, 2020

Choose a reason for hiding this comment

trustin commented Feb 27, 2020

ikhoon commented Feb 27, 2020

ikhoon commented Feb 17, 2020 •

edited

Loading

ikhoon Feb 18, 2020 •

edited

Loading

ikhoon Feb 25, 2020 •

edited

Loading

codecov bot commented Feb 18, 2020 •

edited

Loading

kojilin Feb 26, 2020 •

edited

Loading