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

Feature/jmh+improvements #151

Merged
merged 10 commits into from Mar 3, 2019
Merged

Feature/jmh+improvements #151

merged 10 commits into from Mar 3, 2019

Conversation

OlegDokuka
Copy link
Collaborator

@OlegDokuka OlegDokuka commented Mar 3, 2019

This PR provides the following:

  • bug fix for the case when the assembly hooks are enabled in Reactor so the cancellation may end up exceptionally with NPE
  • provides some fixes in tests
  • improves the performance of AbstractStreamObserverAndPublisher by specifying the lowerTild for next prefetch (it means that we don't have to wait for full prefetch fulfillment so upstream have some space to send, and downstream does not call request to frequently)
  • provides reference JMH benchmarks
  • updates versions of libs

Copy link
Collaborator

@rmichela rmichela left a comment

Choose a reason for hiding this comment

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

I think you can combine the two JMH modules into a single module with one runner. You can generate RxJava and Reactor stubs in the same POM and run both harnesses side by side. This will cut out a bunch of code duplication.

@rmichela
Copy link
Collaborator

rmichela commented Mar 3, 2019

Can you please put bullet points in the PR description for the fixes you made and the dependency upgrades you made?

@rmichela
Copy link
Collaborator

rmichela commented Mar 3, 2019

Failed tests:

clientToServerBackpressure(com.salesforce.rxgrpc.BackpressureIntegrationTest): expected:<[1]L> but was:<[0]L>
serverToClientBackpressure(com.salesforce.rxgrpc.BackpressureIntegrationTest): expected:<[1]L> but was:<[2]L>

@codecov-io
Copy link

codecov-io commented Mar 3, 2019

Codecov Report

Merging #151 into master will increase coverage by 0.96%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #151      +/-   ##
============================================
+ Coverage      47.2%   48.17%   +0.96%     
+ Complexity      116      115       -1     
============================================
  Files            28       28              
  Lines           841      822      -19     
  Branches         81       81              
============================================
- Hits            397      396       -1     
+ Misses          420      401      -19     
- Partials         24       25       +1
Impacted Files Coverage Δ Complexity Δ
...rpc/common/AbstractStreamObserverAndPublisher.java 76.24% <100%> (-0.98%) 49 <3> (-1)
...tivegrpc/common/AbstractSubscriberAndProducer.java 90.65% <100%> (+0.42%) 55 <0> (ø) ⬇️
...n/java/com/salesforce/rxgrpc/stub/ClientCalls.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...n/java/com/salesforce/rxgrpc/stub/ServerCalls.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...a/com/salesforce/reactorgrpc/stub/ServerCalls.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...a/com/salesforce/reactorgrpc/stub/ClientCalls.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...src/main/java/com/salesforce/rxgrpc/GrpcRetry.java 100% <0%> (+12.9%) 0% <0%> (ø) ⬇️
...ain/java/com/salesforce/reactorgrpc/GrpcRetry.java 100% <0%> (+43.75%) 0% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22b8bef...228a951. Read the comment docs.

@OlegDokuka
Copy link
Collaborator Author

@rmichela fixed all comments

@OlegDokuka OlegDokuka requested a review from rmichela March 3, 2019 17:19
@rmichela rmichela merged commit 4e864f3 into master Mar 3, 2019
@rmichela rmichela deleted the feature/jmh+improvements branch March 3, 2019 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants