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

BlockHound support #1682

Merged
merged 2 commits into from
May 4, 2019
Merged

BlockHound support #1682

merged 2 commits into from
May 4, 2019

Conversation

bsideup
Copy link
Contributor

@bsideup bsideup commented May 2, 2019

Add BlockHound SPI's implementation. By keeping it next to core,
we control how we intercept the tasks (e.g., onScheduleHook)
and can also apply internal optimizations in future.

BlockHound's built-in Reactor integration will be adjusted to not
apply anything if Reactor's version is 3.3 or higher.

@bsideup bsideup added the type/enhancement A general enhancement label May 2, 2019
@bsideup bsideup added this to the 3.3.0.M1 milestone May 2, 2019
@codecov-io
Copy link

codecov-io commented May 2, 2019

Codecov Report

Merging #1682 into master will increase coverage by 0.03%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #1682      +/-   ##
===========================================
+ Coverage     84.27%   84.3%   +0.03%     
  Complexity     3932    3932              
===========================================
  Files           361     362       +1     
  Lines         30037   30048      +11     
  Branches       5590    5590              
===========================================
+ Hits          25313   25333      +20     
  Misses         3097    3097              
+ Partials       1627    1618       -9
Impacted Files Coverage Δ Complexity Δ
...r/core/scheduler/ReactorBlockHoundIntegration.java 0% <0%> (ø) 0 <0> (?)
...va/reactor/core/publisher/ParallelMergeReduce.java 69.42% <0%> (-3.31%) 3% <0%> (ø)
.../java/reactor/core/scheduler/ElasticScheduler.java 83.6% <0%> (-1.64%) 26% <0%> (ø)
...c/main/java/reactor/core/publisher/FluxReplay.java 84.31% <0%> (+0.15%) 25% <0%> (ø) ⬇️
...c/main/java/reactor/core/publisher/FluxCreate.java 86.26% <0%> (+0.24%) 8% <0%> (ø) ⬇️
...ain/java/reactor/core/publisher/FluxConcatMap.java 90.14% <0%> (+0.28%) 7% <0%> (ø) ⬇️
...ava/reactor/core/publisher/WorkQueueProcessor.java 70.66% <0%> (+1.23%) 19% <0%> (ø) ⬇️
...in/java/reactor/core/publisher/FluxWindowWhen.java 81.73% <0%> (+1.44%) 2% <0%> (ø) ⬇️
.../java/reactor/core/publisher/BlockingIterable.java 79.16% <0%> (+2.08%) 7% <0%> (ø) ⬇️
... and 1 more

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 3e31271...f494640. Read the comment docs.

Copy link
Member

@simonbasle simonbasle left a comment

Choose a reason for hiding this comment

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

@bsideup I think it should be possible to bump Gradle to 4.10, and use the latest version of the plugin you introduced.

I'm wondering about that plugin, and its possible interactions with pattern-based "test sets" we have so far (loops, tck, tests, jmh)... Maybe either follow a similar pattern-based approach, or migrate everything to unbroken-dome's testSets?

@bsideup
Copy link
Contributor Author

bsideup commented May 2, 2019

@simonbasle

I think it should be possible to bump Gradle to 4.10, and use the latest version of the plugin you introduced.

We're using 4.10 already, and the plugin's second version does not work with 4.10.x. We could try Gradle 5.x, but I remember having issues with upping it.

I'm wondering about that plugin, and its possible interactions with pattern-based "test sets" we have so far (loops, tck, tests, jmh)

There are no conflicts between the test sets (since this is a separate source set anyways), but I would be happy to convert the existing sets if you prefer :)

@bsideup bsideup mentioned this pull request May 3, 2019
@simonbasle
Copy link
Member

@bsideup I'll keep the gitignore commit separate, so I'll do a manual rebase with some squashing before I merge this.

@simonbasle simonbasle force-pushed the add_blockhound_spi branch 2 times, most recently from 38647a5 to ad07981 Compare May 3, 2019 18:05
simonbasle added a commit that referenced this pull request May 3, 2019
Add BlockHound SPI's implementation. By keeping it next to core,
we control how we intercept the tasks (e.g., `onScheduleHook`)
and can also apply internal optimizations in future.

BlockHound's built-in Reactor integration will be adjusted to not
apply anything if Reactor's version is 3.3 or higher.

The integration class must be public to to the ServiceLoader
use, but it is excluded from javadoc and documented inline as
"do not consider public".
The regular expression in the gitignore file was way too broad,
which prevents a SPI file to be committed.
Add BlockHound SPI's implementation. By keeping it next to core,
we control how we intercept the tasks (e.g., `onScheduleHook`)
and can also apply internal optimizations in future.

BlockHound's built-in Reactor integration will be adjusted to not
apply anything if Reactor's version is 3.3 or higher.

The integration class must be public to to the ServiceLoader
use, but it is excluded from javadoc and documented inline as
"do not consider public".
@bsideup bsideup merged commit 4faaa6e into master May 4, 2019
@bsideup bsideup deleted the add_blockhound_spi branch May 4, 2019 07:28
bsideup added a commit to reactor/BlockHound that referenced this pull request May 5, 2019
#29)

Since reactor/reactor-core#1682 is merged now, we should not instrument Reactor because it natively integrates with BlockHound via the SPI
smaldini pushed a commit that referenced this pull request May 6, 2019
Add BlockHound SPI's implementation. By keeping it next to core,
we control how we intercept the tasks (e.g., `onScheduleHook`)
and can also apply internal optimizations in future.

BlockHound's built-in Reactor integration will be adjusted to not
apply anything if Reactor's version is 3.3 or higher.

The integration class must be public to to the ServiceLoader
use, but it is excluded from javadoc and documented inline as
"do not consider public".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants