-
Notifications
You must be signed in to change notification settings - Fork 180
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
Provide JDK flow adapters #824 #904
Conversation
Can one of the admins verify this patch? |
3 similar comments
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
@servicetalk-bot test this please |
__Motivation__ As part of reviewing apple#904 following issues were observed in `ReactiveStreamsAdapters`: - One method Javadoc had a typo linking to the wrong class. - Name of internal classes were not consistently indicating the intent. - Internal class `PublisherToRSPublisher` is redundant, we can instead use `PublisherSourceToRSPublisher` __Modification__ - Fixed javadoc - Renamed classes to follow [Rs, St]To[Rs, St][Publisher, Subscriber, Subscription] format - Inlined a variable in the test. __Result__ Better readability for `ReactiveStreamsAdapters`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @muthupalaniappan for the contribution.
I noticed some minor cosmetic issues with the reactive-streams adapter implementation for which I have created a PR #906 for your reference and have also marked the changes in this PR.
...talk-concurrent-jdkflow/src/main/java/io/servicetalk/concurrent/jdkflow/JdkFlowAdapters.java
Outdated
Show resolved
Hide resolved
...talk-concurrent-jdkflow/src/main/java/io/servicetalk/concurrent/jdkflow/JdkFlowAdapters.java
Outdated
Show resolved
Hide resolved
...talk-concurrent-jdkflow/src/main/java/io/servicetalk/concurrent/jdkflow/JdkFlowAdapters.java
Outdated
Show resolved
Hide resolved
...-concurrent-jdkflow/src/test/java/io/servicetalk/concurrent/jdkflow/JdkFlowAdaptersTest.java
Outdated
Show resolved
Hide resolved
...-concurrent-jdkflow/src/test/java/io/servicetalk/concurrent/jdkflow/JdkFlowAdaptersTest.java
Outdated
Show resolved
Hide resolved
...talk-concurrent-jdkflow/src/main/java/io/servicetalk/concurrent/jdkflow/JdkFlowAdapters.java
Outdated
Show resolved
Hide resolved
...talk-concurrent-jdkflow/src/main/java/io/servicetalk/concurrent/jdkflow/JdkFlowAdapters.java
Outdated
Show resolved
Hide resolved
} | ||
} | ||
|
||
private static final class FlowSubscriber<T> implements PublisherSource.Subscriber<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Rename to StToFlowSubscriber
to match the other class names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should then be FlowToStSubscriber
...talk-concurrent-jdkflow/src/main/java/io/servicetalk/concurrent/jdkflow/JdkFlowAdapters.java
Outdated
Show resolved
Hide resolved
.../src/test/java/io/servicetalk/concurrent/jdkflow/tck/AbstractCompletableOperatorTckTest.java
Outdated
Show resolved
Hide resolved
__Motivation__ As part of reviewing #904 following issues were observed in `ReactiveStreamsAdapters`: - One method Javadoc had a typo linking to the wrong class. - Name of internal classes were not consistently indicating the intent. - Internal class `PublisherToRSPublisher` is redundant, we can instead use `PublisherSourceToRSPublisher` __Modification__ - Fixed javadoc - Renamed classes to follow [Rs, St]To[Rs, St][Publisher, Subscriber, Subscription] format - Inlined a variable in the test. __Result__ Better readability for `ReactiveStreamsAdapters`
Motivation: For interoperability with JDK Flow today we dont have direct flow adapters, users have to use ReactiveStreams adapters and then use the Flow adapter provided by ReactiveStreams which is two step conversion and additional dependency on ReactiveStreams. Modifications: * Added a new module `servicetalk-concurrent-jdkflow` and applied `servicetalk-gradle-plugin-internal-library` * Since flow apis are available only from jdk 9 it gets little tricky here and have to deviate from the standards as below * Build the module only if jdk 9 or above is available during build * Override the source and target compatibility of the module to jdk9 * Added `JdkFlowAdapters` class in main similar to `ReactiveStreamsAdapters` * Added `JdkFlowAdaptersTest` and `reactive-streams-tck-flow` tests in test similar to `ReactiveStreamsAdaptersTest` and `reactive-streams-tck` tests Result: A new module servicetalk-concurrent-jdkflow is added which can be used with jdk9 and above for interoperability with JDK flow api. Fixes apple#824
Motivation: For interoperability with JDK Flow today we dont have direct flow adapters, users have to use ReactiveStreams adapters and then use the Flow adapter provided by ReactiveStreams which is two step conversion and additional dependency on ReactiveStreams. Modifications: * Added a new module `servicetalk-concurrent-jdkflow` and applied `servicetalk-gradle-plugin-internal-library` * Since flow apis are available only from jdk 9 it gets little tricky here and have to deviate from the standards as below * Build the module only if jdk 9 or above is available during build * Override the source and target compatibility of the module to jdk9 * Added `JdkFlowAdapters` class in main similar to `ReactiveStreamsAdapters` * Added `JdkFlowAdaptersTest` test class in test similar to `ReactiveStreamsAdaptersTest` test class Result: A new module servicetalk-concurrent-jdkflow is added which can be used with jdk9 and above for interoperability with JDK flow api. Fixes apple#824
5ce1640
to
fc9bad2
Compare
@servicetalk-bot test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @muthupalaniappan
@muthupalaniappan can you also update the docs to reflect this change now? This is where you need to change: Under "Interoperability", modify the statement:
to
|
Thanks @NiteshKant , As you have already approved it should I make the doc change in a separate PR? |
@muthupalaniappan its ok to do the change in this PR, I can re-review |
Motivation: For interoperability with JDK Flow today we dont have direct flow adapters, users have to use ReactiveStreams adapters and then use the Flow adapter provided by ReactiveStreams which is two step conversion and additional dependency on ReactiveStreams. Modifications: * Added a new module `servicetalk-concurrent-jdkflow` and applied `servicetalk-gradle-plugin-internal-library` * Since flow apis are available only from jdk 9 it gets little tricky here and have to deviate from the standards as below * Build the module only if jdk 9 or above is available during build * Override the source and target compatibility of the module to jdk9 * Added `JdkFlowAdapters` class in main similar to `ReactiveStreamsAdapters` * Added `JdkFlowAdaptersTest` test class in test similar to `ReactiveStreamsAdaptersTest` test class Result: A new module servicetalk-concurrent-jdkflow is added which can be used with jdk9 and above for interoperability with JDK flow api. Fixes apple#824
@servicetalk-bot test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @muthupalaniappan
Motivation:
For interoperability with JDK Flow today we dont have direct flow adapters, users have to use ReactiveStreams adapters and then use the Flow adapter provided by ReactiveStreams which is two step conversion and additional dependency on ReactiveStreams.
Modifications:
servicetalk-concurrent-jdkflow
and appliedservicetalk-gradle-plugin-internal-library
JdkFlowAdapters
class in main similar toReactiveStreamsAdapters
JdkFlowAdaptersTest
test class in test similar toReactiveStreamsAdaptersTest
test class.Result:
A new module servicetalk-concurrent-jdkflow is added which can be used with jdk9 and above for interoperability with JDK flow api.
Fixes #824