eclipse / microprofile-reactive-streams-operators

Microprofile project
Apache License 2.0
80 stars 33 forks source link

Discussion for *Builder naming #6

Closed jroper closed 6 years ago

jroper commented 6 years ago

The current names for the primary classes being used are PublisherBuilder, SubscriberBuilder, ProcessorBuilder and CompletionBuilder. These accurately reflect what they are doing, they are used to build publishers, subscribers, processors and complete graphs. When working with them fluently the names don't even come into play:

Publisher<String> publisher = ReactiveStreams.fromPublisher(somePublisher)
  .map(Object::toString)
  .build();

However, one of the use cases for this library is for APIs to use the interfaces directly, eg, here's how a developer might use the messaging spec:

@Incoming(topic = "mytopic")
public SubscriberBuilder<MyMessage, Void> subscribeToMyMessage() {
  return ReactiveStreams.<MyMessage>builder()
    .forEach(message -> System.out.println("I got a message: " + message));
}

In this case, the Builder name features very prominently in a place that might not be so intuitive.

This issue is to discuss this naming and whether the above is acceptable, or if we should try to come up with different names. The names Publisher, Subscriber and Processor are out due to the confusion it might introduce with Reactive Streams itself, and the fact that imports for the Reactive Streams libraries are already on the classpath.

One option is to go for a completely different set of names, as some other Reactive Streams libraries do. For example, RxJava uses Observable for Publisher and doesn't really have an equivalent for Processor and Subscriber. Reactor uses Flux and Mono for Publisher, and also doesn't have an equivalent for Processor and Subscriber. Akka Streams uses Source, Flow and Sink for Publisher, Processor and Subscriber respectively, though Flow is problematic due to a name class with the JDK9 java.concurrent.util.Flow outer class that holds all the reactive streams interfaces.

hutchig commented 6 years ago

I thought it was interesting that a SubscriberBuilder:build did not return a (Rs)Subscriber but actually returned a SubscriberWithResult. All the other builders are more true to their names :-) I would not push for new names - I am all for one name for one thing and a descriptive name but I have not written as much functional style code as would be needed to have a solid opinion.

I was curious if there was some advantage to Mono and Flux both being subtypes of their Publisher whereas CompletionStage is an interface that has no 'isa' relationship with Flow.Publisher at all - as a discussion point for having 'new' types.

jroper commented 6 years ago

There is certainly an advantage to Mono and Flux both being subtypes of their Publisher, however I'm not sure that in practice it's as big as one might think.

In my experience writing reactive apps, generally there's a hard boundary as to when you want/need an asynchronously provided single value, vs want/need a stream. I don't think I've ever come across a place where being able to treat something as either, or being able to return either from an API, is necessary. I think the primary advantage is that it just makes interoperability between the two a little easier, there's no need to have two variants of flatMap on the publisher for example, and there's no need to provide a mechanism to flatten CompletionStage<Publisher<T>> either.

Another advantage is cross cutting concerns, particularly context passing, can be easily handled consistently across them.

At the same time, there can be advantages between having them separate, since it makes it absolutely clear in an API which one is expected.

All that said, I think the decision has already been made for us. CompletionStage exists as a standard, and has wide adoption in other Java standards such as JAX-RS, the new HTTP client and is also being adopted in ADBA. I'm not sure that it makes sense to introduce a new abstraction for exactly one asynchronous value to be adopted by Java standards. Unfortunately we don't quite have the luxury that Spring has to do our own thing.

Emily-Jiang commented 6 years ago

I have finally checked out the code and took a look.

As for CompletionStage, I think most of the specs will try to return a CompletionStage for any async operations.

As for the builder naming,

  1. PublisherBuilder has buildRs. Why not just build. The builder does not always build a Publisher. Should this be called PublisherFactory as it can create CompletionBuilder as well.

  2. ReactiveStreamBuilder is not a builder and should not be called a builder. I think this class should be called ReactiveStream while the current ReactiveStream should be called ReactiveStreamManager.

  3. SubscriberBuilder should be called SubscriptionBuilder and return a Subscription on its build method.

  4. ProcessorBuilder should be called ProcessorFactory as it creates multiple builders.

jroper commented 6 years ago
  1. The buildRs() naming comes from the approach to reactive doc recommendation for dealing with the transition from org.reactivestreams to java.util.concurrent.Flow. Because this returns an org.reactivestreams.Publisher, at some point in future we will migrate to java.util.concurrent.Flow.Publisher. When that happens, we can introduce a new build() method that returns java.util.concurrent.Flow.Publisher, and keep the buildRs() method for backwards compatibility - that way we don't break backwards compatibility, and we ensure that the final API when juc.Flow is adopted is the nice looking one.
  2. ReactiveStreamBuilder is the parent class of all the builders. It's an implementation detail, we could remove it altogether, it would only save a very small amount of boilerplate in our implementation to keep it.
  3. SubscriptionBuilder is problematic due to there being a Subscription interface in reactive streams itself, and this is not building that.
  4. ProcessorBuilder is a builder for Processor, if you invoke build() (or for now, buildRs()) on it, you get back a Processor. It can become a builder of a different type when you connect a SubscriberBuilder to it, but that's just a changing of shape of the current thing being built, it's not really creating a new builder.
cescoffier commented 6 years ago

Is this issue still valid?

jroper commented 6 years ago

I think it's been adequately addressed.