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

Decouple from ReactiveStreams interfaces #316

Merged
merged 5 commits into from
Feb 19, 2019
Merged

Decouple from ReactiveStreams interfaces #316

merged 5 commits into from
Feb 19, 2019

Conversation

NiteshKant
Copy link
Collaborator

Motivation

With the recent developments in JDK and the introduction of Flow, ReactiveStreams isn't a single standard API.
In the future we may see more adoption of Flow since it is part of the JDK.
In such situations extending from ReactiveStreams becomes a risk such that if everyone uses Flow, ServiceTalk may look like an outdated library.
We should provide our own contract and refer to ReactiveStreams in the documentation to make sure we convey that we are spec compliant.

Modification

  • Introduced PublisherSource as a replacement for RS interfaces.
  • Publisher in concurrent-api now implements PublisherSource instead of RS Publisher
  • Renamed Single and Completable in concurrent module to SingleSource and CompletableSource to match PublisherSource.

Result

No direct dependency on ReactiveStreams.

Copy link
Member

@ddossot ddossot left a comment

Choose a reason for hiding this comment

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

😅🚀

Copy link
Contributor

@jayv jayv left a comment

Choose a reason for hiding this comment

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

Overall makes sense, as discussed, some question around references and imports / line length issues.

* successfully or with an error.
* <p>
* This is a replica of the APIs provided by
* <a href="https://github.com/reactive-streams/reactive-streams-jvm">Reactive Streams</a> and follows the
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make reference to reference JDK Flow as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

JDK Flow is a replica of ReactiveStreams and does not do a good job in referencing to the specifications. I am not sure there is value in doing that now, we can update documentation later if required.

Copy link
Contributor

Choose a reason for hiding this comment

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

With the recent developments in JDK and the introduction of Flow, ReactiveStreams isn't a single standard API. [...]

...we start the PR with the above msg and then never mention Flow at all, strange.

JDK Flow is a replica of ReactiveStreams and does not do a good job in referencing to the specifications

1st paragraph of j.u.c.Flow API [1] links to the spec:

These interfaces correspond to the reactive-streams specification.

If we're assuming Flow will become a more predominant bridge interface, what is our strategy regarding implementing/converting the Flow.* types?
Do we expect the user to provide adapters or will we provide these once we also target JDK 11 ?

[1] https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/concurrent/Flow.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we start the PR with the above msg and then never mention Flow at all, strange.

The motivation for this change is that there are more than one SPIs now in this space, hence the mention of Flow which is the second SPI after reactive streams.

me:

does not do a good job in referencing to the specifications.

@jayv

1st paragraph of j.u.c.Flow API [1] links to the spec:

Flow does not do a good job in referencing to the spec because it mentions the spec in the parent class(Flow) but does not get specified anywhere on the actual interfaces (Publisher, Subscriber, Subscription, Processor). To make the lack of reference to the spec more problematic, it defines some rules/behavior on the interfaces again w/o referring to the spec rules. Due to these reasons, I feel referring to Flow isn't really adding value here in our docs (if not making it ambiguous). We are basically following reactive streams for spec and TCK. Since, this is just documentation, we do not really have to be exhaustive in which contracts are similar to RS but just what we are following, which is reactive streams.

If we're assuming Flow will become a more predominant bridge interface,

We are not assuming Flow to be more predominant but we are not assuming the reverse too. This change is just acknolwedging the fact that there are more than one SPIs existing now hence making the choice to stick to one of them (API wise) is risky. Having our own contract for the sources and then providing adapters for all potential SPIs reduces that risk.

Do we expect the user to provide adapters or will we provide these once we also target JDK 11 ?

Reactive streams provide reactive streams to flow adapters today[1] which can be used by users but it would require two step adaptations (ST types -> RS types -> Flow). Once we require JDK 11, then we can provide direct adapters to Flow.

[1] https://github.com/reactive-streams/reactive-streams-jvm/blob/master/api/src/main/java9/org/reactivestreams/FlowAdapters.java

@NiteshKant
Copy link
Collaborator Author

@jayv I have scanned all files for > 120 and unnecessary qualifiers for package names.

@NiteshKant NiteshKant requested a review from jayv February 18, 2019 21:41
Nitesh Kant added 5 commits February 19, 2019 11:01
__Motivation__

With the recent developments in JDK and the introduction of Flow, ReactiveStreams isn't a single standard API.
In the future we may see more adoption of Flow since it is part of the JDK.
In such situations extending from ReactiveStreams becomes a risk such that if everyone uses Flow, ServiceTalk may look like an outdated library.
We should provide our own contract and refer to ReactiveStreams in the documentation to make sure we convey that we are spec compliant.

__Modification__

- Introduced `PublisherSource` as a replacement for RS interfaces.
- `Publisher` in concurrent-api now implements `PublisherSource` instead of RS `Publisher`
- Renamed `Single` and `Completable` in concurrent module to `SingleSource` and `CompletableSource` to match `PublisherSource`.

__Result__

No direct dependency on ReactiveStreams.
@NiteshKant NiteshKant merged commit 3c25794 into apple:master Feb 19, 2019
@NiteshKant NiteshKant deleted the no-rs branch February 19, 2019 21:17
Copy link
Member

@Scottmitch Scottmitch left a comment

Choose a reason for hiding this comment

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

quick glance ... lgtm

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

5 participants