1.1.0 - Changed javadoc for Observable.doOnRequest(Action1)

[stealthcode]

This is one of the many 1.1.0 promotion related pull requests. There is a split decision on the operator Observable.doOnRequest(Action1). A majority of core committers have voted to promote this operator from @Beta to public. There is currently a minority is support for removing the convenience method on Observable or demoting it to @Experimental. Instead users would lift the underlying operator obs.lift(new OperatorDoOnRequest<T>(onRequest)). In this pull request I have taken the more conservative approach and expect that comments will guide our decisions.

Rationale:

• the doOnRequest use case is to debug back-pressure use cases and as such should not pollute the public operator namespace.
• the existence of this operator may mislead and confuse users. it could be misinterpreted and abused to alter or reset over arching state when the back-pressure mechanics should be more or less self contained (with the exception of AsyncOnSubscribe).

If my understanding is incorrect and there is a valid use case aside from debugging then please comment.

[davidmoten]

I'd like to see this method part of the public api not because of a
debugging requirement but rather for unit testing. I think that's enough.

On Wed, 30 Sep 2015 03:37 Aaron Tull [email protected] wrote:

This is one of the many 1.1.0 promotion related pull requests. There is a
split decision on the operator Observable.doOnRequest(Action1). A
majority of core committers have voted to promote this operator from @beta
to public. There is currently a minority is support for removing the
convenience method on Observable or demoting it to @experimental. Instead
users would lift the underlying operator obs.lift(new
OperatorDoOnRequest(onRequest)). In this pull request I have taken the
more conservative approach and expect that comments will guide our
decisions.
Rationale:

• the doOnRequest use case is to debug back-pressure use cases and as
  such should not pollute the public operator namespace.
• the existence of this operator may mislead and confuse users. it
  could be misinterpreted and abused to alter or reset over arching state
  when the back-pressure mechanics should be more or less self contained
  (with the exception of AsyncOnSubscribe).

If my understanding is incorrect and there is a valid use case aside from

debugging then please comment.

You can view, comment on, or merge this pull request online at:

#3386
Commit Summary

• Changed Observable.doOnRequest(Action1) to @experimental from @beta

File Changes

• M src/main/java/rx/Observable.java
  https://github.com/ReactiveX/RxJava/pull/3386/files#diff-0 (2)

Patch Links:

• https://github.com/ReactiveX/RxJava/pull/3386.patch
• https://github.com/ReactiveX/RxJava/pull/3386.diff

—
Reply to this email directly or view it on GitHub
#3386.

[artem-zinnatullin]

Can we put something like "Operator was created for testing/debugging RxJava itself, using it in the production code is not very great idea" in the javadoc of this operator?

[stealthcode]

@davidmoten could this be solved by lifting in the operator? I don't think that we should polute theObservable instance methods for a convenience operator that's intended to be used by unit tests.

[akarnokd]

This is a useful operator to diagnose operator request behavior and should be part of the public API in my opinion. Similar to how TestScheduler and TestSubscriber isn't useful in production but very useful when testing code. Pointing the customers to the internal OperatorDoOnRequest class for manual lifting isn't a good alternative because of forcing the customers to rely on internal behavior.

[davidmoten]

I'd recommend against having public API exposing an Operator but would use a Transformer instead. I see moving this out of the Observable class as an unnecessary inconvenience. I haven't come across anyone doing weird stuff using doOnRequest. Have you @stealthcode? Can we address this with clearer documentation on the operator?

[abersnaze]

Pointing the customers to the internal OperatorDoOnRequest class for manual lifting isn't a good alternative because of forcing the customers to rely on internal behavior.

The point is that request(n) should remain an internal only affair and if you need to know about requests that you'll probably also need to know how to lift in operators.

[abersnaze]

@davidmoten I don't see how o.lift(new OperatorDoOnRequest(System.out::println)) is different than o.compose(new TransformerDoOnRequest(System.out::println)).

[akarnokd]

This operator is a great tool for diagnosing errors in custom operators, which we support through lift and Operator classes.

[davidmoten]

@abersnaze Yeah I should have elaborated. Either could be used.

In general I was thinking of utility from #2865 which doesn't exist in RxJava but could. I can contribute. Until this utility exists I wouldn't suggest anyone outside of RxJava builds a public API exposing custom Operators because they will have trouble migrating safely if they decide to chuck their custom Operator and switch to composing existing Operators (I had this issue with rxjava-jdbc). This advice really doesn't apply for core RxJava because as soon as the need exists someone with the right skills would create it with oversight. So yeah ignore that comment of mine, it's irrelevant to this discussion!

[stealthcode]

@akarnokd We agree that it's useful and we are not proposing to remove the OperatorDoOnRequest. But its existence as an instance method on Observable is questionable. It is very useful for RxJava developers but confusing for application developers. Do you agree with this?

[akarnokd]

I personally never heard anybody getting confused by doOnRequest so I can't agree or disagree with your statement.

[stealthcode]

So, what is your objection?

[akarnokd]

My objection is that by demoting this to experimental, we are essentially scheduling it for deletion, despite all of its usefulness.

[stealthcode]

It is not less useful as an operator that can be lifted. It's only use case is for developers who know how to lift operators.

[akarnokd]

I'd like to understand the source of any confusion with this method. Could you invite someone who is/was confused into this discussion?

[davidmoten]

It is not less useful as an operator that can be lifted. It's only use case is for developers who know how to lift operators.

That's all very well but we lose discoverability. For anyone discovering the api no matter how advanced their abilities and intent I think it's a bad idea to have this only as an Operator to be lifted that only we know about and some corner bit of documentation describes.

[benjchristensen]

I prefer keeping it and like using it. I use it for debugging but also triggering side-effects onRequest, rather than onSubscribe.

[stealthcode]

Thanks @benjchristensen that's the first non-test/debug use case mentioned thus far.

We currently have 2 for and 2 against (core committer votes).

[stevegury]

I don't have a strong opinion here; I think both points of view are valid.
doOnRequest can be confusing for a new user and it can be interpreted as doOnNext by beginners. And in general, the more methods you add to a class, the harder it is to understand it.
On the other hand, having the possibility to hook easily (by just auto-completing obs.doOnRequest(...) a function in the request-n path is pretty handy.

So the question here is: Do we want to make the library marginally more grokkable at the expense of the power-users?

Maybe the right thing to do would be to leave it as @beta?

[stealthcode]

What do you think of adding the following to the javadocs of the Observable.doOnRequest(Action1) something to the effect of...

This operator is for tracing the internal operator back-pressure request patterns 
and generally intended for debugging. 

This would give users some clarity and scare off all but the intrepid and curious.

[benjchristensen]

I'm okay with adding that kind of Javadoc.

[davidmoten]

Javadoc additions sound fine to me too.

[stevegury]

@stealthcode that would be a good compromise.

[stealthcode]

To be clear, I am proposing that the only change to this operator (in 1.1.0) is only a javadoc change and we keep the annotation as is at @Beta.

[abersnaze]

Looks like we have a consensus merging this one and closing #3405