Skip to content
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

API design #1

Merged
merged 68 commits into from Sep 3, 2019
Merged

API design #1

merged 68 commits into from Sep 3, 2019

Conversation

cescoffier
Copy link
Contributor

No description provided.

@emmanuelbernard
Copy link

You still have in UniSubscribe the inconsistent CompletableFuture asCompletionStage()

@cescoffier
Copy link
Contributor Author

Why is it inconsistent? You mean it should be adapt().asCompletionStage() ?

@emmanuelbernard
Copy link

Katia feedback:

  • await is used everywhere else and probably could be less disrupting
  • "ca va loin cette API, mais ça fait plaisir :)"
  • what's the difference between or() and onAny() (Emmanuel)

@cescoffier
Copy link
Contributor Author

cescoffier commented Jul 24, 2019

or vs. any that's a great question and you can ask the same about and vs. zip.

and and or are instance methods. You combine the current Uni with another one.
any and zip are static methods. You pass all participants.

Now I'm not happy with this, and I'm thinking about moving zip and any into createFrom, but it does not "fit" well there either...

@emmanuelbernard
Copy link

yeah

@kenfinnigan
Copy link
Collaborator

What about instead of Uni.any().of(...)and Uni.zip().unis(...) it becomes Any.uni().of(...) and Zip.unis(...)?

Then for Multi it would be Any.multi().of(...) and Zip.multis(...).

@kenfinnigan
Copy link
Collaborator

If we don't need to support Iterable as an argument to of(), it could be shortened to Any.of(...)

* @throws RuntimeException if the conversion fails.
*/
public <O> O to(Class<O> clazz) {
return new UniAdaptTo<>(upstream, nonNull(clazz, "clazz")).adapt();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the advantage to returning a new instance here instead of having some of the code that the class has around returning if it's the same thing, or returning a completion stage, and only handle actual adaptation in UniAdaptTo?

Doesn't appear to be used anywhere else, so curious

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that can definitely be static.

This optimization can be done in a few places where we could pass the upstream as a parameter, and so avoid the useless instantiation. To be honest I didn't focus on this so far.

@Ladicek
Copy link
Contributor

Ladicek commented Jul 25, 2019

Re Uni.any and Uni.zip -- I'd just call them Uni.anyOf and Uni.allOf, similarly to CompletableFuture.

@Ladicek
Copy link
Contributor

Ladicek commented Jul 25, 2019

So I'd like to highlight one concern I have with the proposed API. It's probably been decided already, but I'm gonna complain anyway :-)

I really don't like how different parts of the API are split into different pieces, presumably to make it easier to navigate, understand, more fluent, etc. I very much disagree with this reasoning, because:

  1. It actually makes the API harder to understand and use, both for beginners and experts. Compared to e.g. RxJava types or java.util.stream.Stream, where the entire API is mostly contained in a single type, here, it's scattered around multiple ones. That probably doesn't matter if you expect people to copy incantations from examples and guides, but it becomes a hindrance very quickly when you start writing code from scratch. As a beginner, I often search for the method I need by skimming the entire API and looking mainly at types (types are a front line of documentation!). With the API split into multiple pieces, I can no longer use types as a guidance and I'm left with method names. As an expert, I already know what I want and having to chain multiple calls is just a nuisance (why do I need to type more?).

    Granted there are some places in the API where fluent API is actually better than a flat one. But uni.onResult().mapToResult(...) / uni.onResult().mapToUni(...) is a complete abomination compared to uni.map(...) / uni.flatMap(...) (I personally hate the flatMap name, but it's better than bind :-) and seems pretty well established in the industry). As another example, uni.await().indefinitely() / uni.await().atMost(long, TimeUnit) is still worse than uni.await() / uni.await(long, TimeUnit) (which is a very recognizable pattern from the JDK APIs), albeit not as much as the map / flatMap thing.

    In short -- IMHO this decision makes API discoverability significantly worse.

  2. With reactive types, the less intermediate results you can create and hold on to, the better. In my experience, if it's possible to write everything as a single call chain, it's best. There's nothing wrong with just creating temporary variables holding intermediate state of the pipeline creation, but it's tempting to start calling methods on them, which easily ends up badly.

  3. Established libraries don't do this. Being different from established libraries is fine, but the "innovation budget" is only so big, so we better spend it wisely. This aspect is IMHO relatively unimportant and being consistent on this lets us be different where it's important.

  4. If you argue with separation of concerns, I argue with principle of locality!

If you've made it here, thanks a lot! We don't have to agree, but I believe this just had to be expressed loudly :-)

@FroMage
Copy link
Contributor

FroMage commented Aug 23, 2019

About the deferred prefix. It' actually a question I had for you. What makes more sense to you:

  • Uni.createFrom().item(() -> x) - you pass a supplier, it's called at subscription time
  • Uni.createFrom().deferredItem(() -> x) - same as previous by explicitly mention the lazyness

I think a parameter of X means eager and Callable<X> means lazy. I don't think there's a need to have both variants, no?

When you retrieve a CS, you are requesting the result, so you subscribe to the Uni. Basically, the CS is your subscriber.

OK, got it. Perhaps this would warrant a shortcut, though.

Rename receiveSubscriptionOn to subscribeOn
Implement Multi.emitOn and Multi.subscribeOn
@cescoffier
Copy link
Contributor Author

I think a parameter of X means eager and Callable means lazy. I don't think there's a need to have both variants, no?

The idea was to be more explicit in the name.

Initially [taking the item method as exmaple], I add item(T item) and item(Supplier<T> supplier) - so the lazy aspect only described in the javadoc. As it may have been misleading, I renamed to deferredItem. If you think it's not required, happy to drop the prefix!

OK, got it. Perhaps this would warrant a shortcut, though.

Yes, I agree. Created #20 to track this.

@cescoffier cescoffier mentioned this pull request Aug 27, 2019
Add multi.subscribe().with(...) accepting various callbacks.

SmallRye Reactive Operators reuse and even rely on Reactive Streams.
Some of the events are directly mapped from Reactive Streams _signals_.
But, only `Multi` implements the Reactive Streams `Publisher`.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the advantages of the Reactor classes is that they have a common superclass that can be used
elsewhere so that things that can be plugged onto either only have to be written once to the common interface.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will read the whole proposal carefully, the above is just an intial thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a common interface can be tricky. Typically, it cannot be Publisher, because, on Uni we don't want to be a Publisher (we can be wrapped into one). This allows supporting null, for operation not providing an item.

So instead of having a common interface, I choose to provide a way to convert one to the other one (Uni -> Publisher, Multi -> Uni [potentially dropping elements])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I'm not sure this files is up to date....

@cescoffier cescoffier marked this pull request as ready for review September 3, 2019 12:05
@kenfinnigan kenfinnigan merged commit 9e5926c into master Sep 3, 2019
@kenfinnigan kenfinnigan deleted the initial-work branch November 5, 2019 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants