Skip to content

v2 work-in-progress #3201

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

Closed
benjchristensen opened this issue Aug 26, 2015 · 14 comments
Closed

v2 work-in-progress #3201

benjchristensen opened this issue Aug 26, 2015 · 14 comments

Comments

@benjchristensen
Copy link
Member

Since @akarnokd has started sprinting on v2 ... a place to discuss back-and-forth ...

@benjchristensen benjchristensen added this to the 2.0 milestone Aug 26, 2015
@benjchristensen
Copy link
Member Author

@akarnokd I'm pulling in the snapshot and starting to apply it to a project.

Things I still need to replace 1.x are:

  • doOnSubscribe
  • doOnUnsubscribe
  • concatWith
  • mergeWith
  • publish()
  • ConnectableObservable .connect
  • TestSubscriber
  • subscribe overloads (to use lambdas)

Are you working on any of these or have them close? Otherwise I'll start on some of them.

@benjchristensen
Copy link
Member Author

The project I want to use v2 with is this: https://github.com/ReactiveSocket/reactivesocket-java

It uses Reactive Streams as the public API, and is very performance sensitive. Right now I'm only using RxJava v1 in the unit tests, and the operators/classes listed above are what I need before I can move from v1 to v2.

To use in the main code and replace the custom Publisher impls will require v2 proving it can be added without impacting performance.

We also can't go too much farther with v2 work without establishing how our unit and perf tests will be done.

@akarnokd
Copy link
Member

These are available:

  • publish()
  • ConnectableObservable (except refCount())

I plan to do concatMap next in the morning. If you really need it, you can do mergeWith via merge, but I plan to implement a version that compacts chains of mergeWith calls.

The other operators you mentioned are lower priority for me and plan to leave them for now.

Otherwise, as of now 2.x contains:

  • complete Scheduler API and Schedulers
  • source operators (just, fromX, range, empty, error, never)
  • map, flatMap, filter, merge, mergeDelayError
  • take (untimed), skip (untimed), takeUntil
  • observeOn, subscribeOn, unsubscribeOn
  • all overloads of publish, cache, replay
  • lift, compose, to
  • any, all, count, single, elementAt, isEmpty
  • toList, toSortedList

I plan to move the unit tests one by one after all operators have been implemented. As for the perf classes, I thought adding RxJava 1.0 as a perf dependency to gradle but don't know how (perfCompile ?). That way, comparative benchmarks can be established within 2.x.

@artem-zinnatullin
Copy link
Contributor

@benjchristensen @akarnokd since RxJava v2 will be different from v1 in many aspects I want to ask: you still want to keep both Subscriber and Observer?

It adds some confusion for RxJava users that I know (and for me in past, but then I've found #792).

See:

Or if you still want to keep both of them can we please add more information and notice the difference between Observer and Subscriber in javadoc and make it more visible on reactivex.io? (there is a paragraph about Unsubscribing on the page about Observable)

@benjchristensen
Copy link
Member Author

@artem-zinnatullin if more docs would help, perhaps you could help contribute them in https://github.com/ReactiveX/reactivex.github.io?

It's too early in 2.x to have the Observer/Subscriber details finalized, but I expect we'll still have a similar division. The org.reactivestreams.Subscriber represents the backpressure and unsubscribe capable consumer, while io.reactivex.Observer could be the simpler consumer that doesn't support unsubscribe, and requests Long.MAX_VALUE (the normal default for a synchronous consumer). This distinction still makes sense to me.

In 2.x however we should no longer need a concrete Subscribe with SafeSubscriber semantics or unsubscribe semantics on termination.

@benjchristensen
Copy link
Member Author

@akarnokd thank you for the info

I plan to move the unit tests one by one after all operators have been implemented. As for the perf classes, I thought adding RxJava 1.0 as a perf dependency to gradle but don't know how (perfCompile ?). That way, comparative benchmarks can be established within 2.x.

Before moving the unit tests over, I want us to agree on what the base tests for each operator should be. The 1.x unit tests all organically happened over 2+ years and our experience with what should be expected on each operator changed dramatically over that time. Let's capture that experience and knowledge in a base test suite that each operator must pass. Additionally, each operator should pass the Reactive Streams TCK.

Once we have that, then I'm okay with us moving over other tests to assert specific operator behavior outside the standard Reactive Stream/ReactiveX semantics.

As for perf tests, that's an interesting idea to have 1.x as a dependency for comparisons. Yes, I think it would be perfCompile. Similar to unit tests, it would be good to have a base suite of perf tests for an operator.

@benjchristensen
Copy link
Member Author

Here are the issues applicable to unit tests and perf:

@akarnokd
Copy link
Member

@artem-zinnatullin In the current 2.x, Observer is an abstract class that by default requrests Long.MAX_VALUE but you can override this behavior and gives the ability to cancel. It resembles the 1.x Subscriber behavior as it is stateful. The added benefit to 2.x is that since it extends RS Subscriber, you can have your Observer subscribed to any Publisher, not just RxJava 2.x Observables.

@benjchristensen I don't know much about the RS TCK but it is a good opportunity for the community to submit PRs that uses it.

@akarnokd
Copy link
Member

@benjchristensen All your requested methods/classes have been added to 2.x.

@benjchristensen
Copy link
Member Author

I don't know much about the RS TCK but it is a good opportunity for the community to submit PRs that uses it.

Yes, or I'll get to it. My ask is that you don't start on the unit tests until we've agreed upon an approach.

All your requested methods/classes have been added to 2.x.

You're a machine! :-) Playing with them shortly ...

@artem-zinnatullin
Copy link
Contributor

You're a machine! :-)

+100!

@benjchristensen @akarnokd thanks for explanation about Observer's & Subscriber's future. I'll think about better docs for v1 and I'll try to show the difference between them for the users on my talk about RxJava.

@akarnokd
Copy link
Member

akarnokd commented Sep 9, 2015

I've finished with the test porting; all tests I considered relevant are now ported and pass.

There are, however, about ~80 ignored tests and ~20 commented out which either test for null or throws from one of the onXXX methods which is not allowed by the RS spec. In addition, I've placed ~200 FIXMEs and a bunch of TODOs. These mostly require confirmation for deletion, or rarely, behavior adjustments if we don't want to stray away to much from 1.x in those corner cases.

I haven't copied over the javadocs. This honorable task can go to anybody that wishes to do it.

I haven't added unit test for any new behavior, operator or overload nor are there explicit null-tests. This can also happen later once the feature-set reviews are complete.

Last of all, I'm not particularly interested in Single and the non-backpressure Observable NbpObservable. I've implemented some example operators on them to establish a pattern.

This means 2.x can now switch to the regular review-process (famous last words before a critical bug is discovered that takes weeks to be merged... :) ). All PRs targeting 2.x will now be subject to the same 2 approvers rule.

@benjchristensen
Copy link
Member Author

Excellent work @akarnokd

Thanks for confirming that we'll now flip to the PR review model.

Now that things have calmed down I will try using 2.x again and provide feedback as I have it. @headinthebox and I have been debating some design things related to Flowable/Observable/Single that I hope to get some clarity on in the coming week or two and will share for discussion. As that discussion matures, I will help with the NbpObservable design work.

@akarnokd
Copy link
Member

Closing this in favor of #4029.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants