2.x: Design.md +extension +fusion #3980

akarnokd · 2016-06-01T07:45:13Z

Added sections from #3935.

stealthcode · 2016-06-01T17:39:28Z

DESIGN.md

-Observable<T>.create(OnSubscribe<Observer<T>> onSubscribe)
+Therefore, new standard factory methods will try to address the common entry point requirements: 
+  - `create(SyncOnSubscribe)` to safe, synchronous generation of signals, one-by-one
+  - `create(AsyncOnSubscribe)` to batch-create signals based on request patterns (I'm still not convinced)


I'm still not convinced

I don't think personal opinions belong in a design doc. Can you add a comment or try to articulate this outside of the committed document?

stealthcode · 2016-06-01T20:51:56Z

DESIGN.md

+
+| Method | Flowable | Observable | Single | Completable |
+|--------|----------|------------|--------|-------------|
+| `create(SyncGenerator<T, S>)` | Yes | Yes | No | No |


A SyncGenerator (the SyncOnSubscribe) could be used to model a Single or a Completable.

SyncGenerator allows calling onCompleted which is invalid for Single and onNext which is invalid for Completable.

The CompletionEmitter can only model an empty observable but you list it with the others. Could you please remove the table and just list how you propose that people will create each type? I don't think that these methods are directly comparable in a tabular format.

Since Flowable, Observable can also be empty, you can trigger completion with all 3 types.

Flattened the table.

stealthcode · 2016-06-01T21:06:55Z

@akarnokd since the edits are causing all in-line comments to be hidden I'm posting our discussion on "extension" functionality here.

compose() is different from extend because it gives the full base type to the function. Substituting R with Flowable the signature looks like this:
public final <U> Flowable<U> compose(Function<? super Flowable<T>, ? extends Publisher<U>> composer);

Okay, I would rather only have one method for "extending" functionality? Is that also your goal? In which case we should discuss the merits of compose as a replacement for extend.

For comparison I'm including the signature of extend.

<O2, X extends Consumable<O2>> X extend(Function<Consumer<O>, X> f)

Which in the case of a Flowable would be as follows (note that I have replaced the Consumable<O2> with Publisher<U>)

<O2, X extends<Publisher<U>> X extend(Function<Consumer<Subscriber<? super T>>, X> f)

So the only substantial difference between compose and extend is that with compose the function consumes a concrete Flowable type and with extend it consumes an OnSubscribe.

I think my concern is that the compose functions must know the absolute type of the thing that you are converting from (or at least a base class/interface). This makes it more restrictive than what extend allows, converting from any variation of a Flowable by virtue of unwrapping and accessing the internal callback. So composition functions would be case specific, and would not be reusable for different types of flowable, observable, single, or completable.

Note that I am NOT proposing making conversion functions from a flowable that also work on an observable for instance. What I am proposing is that if you have different variations of an Observable that the same logic could be use to convert it to a different thing. I think that there is the potential for better code reuse with extend. Also my intent is that extending functions for the core RxJava types (i.e. the Flowable, Observable, Single, Completable and all of there variants which implement their respective Consumable<S> for their subscriber/observer type) be written and packaged with RxJava. But this would not be limited to just internal apis as the pattern could be useful for libraries which write their own Consumable.

stealthcode · 2016-06-01T22:23:45Z

DESIGN.md

+
+    @Override
+    protected void subscribeActual(Subscriber<? super R> subscriber) {
+        source.subscribe(new FlowableMapSubscriber<T, R>(subscriber, mapper));


Should this be source.subscribeActual?

That would bypass the plugin hook in source.

Like @stealthcode, I was under the impression that we had decided to go with the Consumable interface.

If you implement it, then go with Consumable.

That's not how collaborating on a team works @akarnokd.

stealthcode · 2016-06-04T00:48:20Z

My take away from this pull request is that @akarnokd has decided on a plan without communicating it to the other contributors on the project. I would like to hear much more details on how this is to be implemented and I don't think that this PR is the right way to do so. I would prefer to have much better communication with the RxJava Core Committers so that we can try to reach some sort of understanding of our motivations and goals in design.

We at Netflix have tried to communicate with a proof of concept and many conversations over PRs and issues to no avail. It seems to me that there are a very significant misalignments in the goals and rationale behind design decisions.

For example, I personally do not see a significant benefit in complicating the code to avoid a minority of memory allocations. I am not considering removing the Operator interface to save a 1 time allocation. The chances are very good that the JVM will optimize the runtime anyway so multiple allocations wouldn't be a significant overhead.

akarnokd · 2016-06-05T14:44:15Z

The last 9+ months was quite tedious for me regarding this project and Netflix' behavior: extended periods of inactivity, delayed decision making, extensive gating of contributions and being stuck in local design/thinking choices expressed repeatedly (or argued endlessly) by some. In addition, I simply don't have the capacity give tailored explanations to everybody.

Such local thinking is the negligence of allocation count. It might not be an issue on multi-gigabyte Amazon cloud servers, but RxJava has to run on millions of Android devices where memory footprint and allocation does matter.

The proof-of-concepts Netflix provided in this regard were too small to be conclusive; they lacked some non-trivial operators and usage examples, with which the compiler ambiguities or coding inconveniences could have been discovered - apparently, me pointing them out is not enough.

Note that there has been a working RxJava 2.x implementation available as developer preview since September 2015 in the 2.x branch here. Its review never progressed even before this design document was posted by Netflix.

As the primary contributor and maintainer of RxJava, I had to start making decisions on my own due to these. The basis for it is the fact that I've been developing complete reactive libraries before and during my time with RxJava and accumulated considerable amount experience with all things reactive.

The design choices I made for RxJava 2.x are proven in the research project Reactive-Streams-Commons and in fact forms the basis for the competing library Reactor-Core of Pivotal. Textual descriptions on why can be found in my blog: RxJava Design Retrospect, Operator Fusion, part 1 and part 2.

There are two reasons I'm still contributing to RxJava: 1) it's the only proper reactive library for Android and 2) Reactor-Core decided to provide only a subset of operators. However, any of this can change.

In conclusion, all I can say is to take this PR, propose alterations to it in a separate PR or leave it.

stealthcode · 2016-06-08T19:51:24Z

I think RxJava core committers team has a lack of cohesion on 2.x designs because there is a lack of communication. @akarnokd we have proposed changes in our comments but you have no patience with our concerns. We have tried reaching out to you by email, instant message, and video chat in the past but you don't seem interested in direct communication outside of Github. Being a part of a team does require a higher level of coordination and communication however I suspect that you have no intention of working with us.

run on millions of Android devices where memory footprint and allocation does matter.

Our goal is to develop a generally applicable library that can be safely and productively used by Android as well as other platforms. @stevegury and I believe that the JVM runtime (even on Android) would optimize the code such that there is little to no difference between the source code optimized implementation and our proposed designs. I think its in the best interest of the RxJava community for the contributors to evaluate different options.

My professional work at Netflix uses and depends heavily on RxJava. I try to contribute my professional expertise to RxJava however when I read responses from @akarnokd I perceive a tone of hostility and a lack of interest. Many others at Netflix choose not to contribute to RxJava because it is too difficult to make progress on pull requests. I have not found an efficient way to collaborate with @akarnokd in design or implementation. RxJava is not benefiting in significant ways from Netflix contributions as many of our pull requests are deadlocked in debate or discouraged.

There are aspects that we don't understand about this approach and when we reach out to him we are met with what I interpret as avoidance and hostility. I can only conclude that @akarnokd is trying to implement what he thinks is best for the community while avoiding any contribution from Netflix.

akarnokd · 2016-06-17T08:28:57Z

Since this is a design document change only, I'm merging this as is, allowing progress on the code side as well as enabling PRs for further clarifications if necessary.

I encourage contributors with architectural ideas to make sure their suggested changes actually work out in the context of the entire 2.x codebase. The current 2.x branch contains a working (although outdated) variant and let's anyone experiment within his/her own environment.

akarnokd added the Documentation label Jun 1, 2016

stealthcode reviewed Jun 1, 2016
View reviewed changes

2.x: Design.md +extension +fusion

904c198

akarnokd force-pushed the DesignPlus branch from e2c9d2b to 904c198 Compare June 1, 2016 20:40

stealthcode reviewed Jun 1, 2016
View reviewed changes

akarnokd added 3 commits June 1, 2016 23:24

Flatten the create-table

a8afb07

Update create naming to avoid overload conflicts

d8a716e

Add missing explanation, remove words

cd17a20

stealthcode reviewed Jun 1, 2016
View reviewed changes

akarnokd merged commit 12490fd into ReactiveX:2.x Jun 17, 2016

akarnokd deleted the DesignPlus branch June 17, 2016 08:29

2.x: Design.md +extension +fusion #3980

2.x: Design.md +extension +fusion #3980

Uh oh!

Conversation

akarnokd commented Jun 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stealthcode commented Jun 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stealthcode commented Jun 4, 2016

Uh oh!

akarnokd commented Jun 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stealthcode commented Jun 8, 2016

Uh oh!

akarnokd commented Jun 17, 2016

Uh oh!

Uh oh!

akarnokd commented Jun 5, 2016 •

edited

Loading