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

fix #1408 Add ability to multi-decorate backing ExecutorServices #1409

Merged
merged 3 commits into from
Nov 20, 2018

Conversation

simonbasle
Copy link
Member

@smaldini I wonder if the decorateExecutorService method at the Schedulers level should be made accessible to outside implementors of Scheduler (since outside implementations would need to call it in order for the decorators to be applied to internals)

@codecov-io
Copy link

codecov-io commented Oct 25, 2018

Codecov Report

Merging #1409 into master will increase coverage by 0.13%.
The diff coverage is 81.25%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1409      +/-   ##
============================================
+ Coverage      84.2%   84.34%   +0.13%     
- Complexity     3869     3896      +27     
============================================
  Files           357      357              
  Lines         29349    29509     +160     
  Branches       5452     5495      +43     
============================================
+ Hits          24712    24888     +176     
+ Misses         3035     3027       -8     
+ Partials       1602     1594       -8
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/reactor/core/scheduler/Scheduler.java 100% <ø> (ø) 5 <0> (ø) ⬇️
...java/reactor/core/scheduler/ParallelScheduler.java 78.26% <100%> (ø) 24 <0> (ø) ⬇️
.../java/reactor/core/scheduler/ElasticScheduler.java 85.12% <100%> (+1.51%) 26 <0> (ø) ⬇️
...actor/core/scheduler/DelegateServiceScheduler.java 60.97% <100%> (ø) 11 <0> (-1) ⬇️
...n/java/reactor/core/scheduler/SingleScheduler.java 76.59% <100%> (-0.49%) 17 <1> (ø)
...c/main/java/reactor/core/scheduler/Schedulers.java 84.05% <76.92%> (+2.23%) 74 <9> (+16) ⬆️
...main/java/reactor/core/publisher/FluxGenerate.java 69.32% <0%> (-0.62%) 7% <0%> (ø)
...c/main/java/reactor/core/publisher/FluxReplay.java 84.16% <0%> (-0.16%) 25% <0%> (ø)
...ain/java/reactor/core/publisher/FluxConcatMap.java 90.14% <0%> (+0.28%) 7% <0%> (ø) ⬇️
...ava/reactor/core/publisher/WorkQueueProcessor.java 69.83% <0%> (+0.41%) 19% <0%> (ø) ⬇️
... and 13 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 a2d1dc5...ea26261. Read the comment docs.

static ScheduledExecutorService decorateExecutorService(String schedulerType,
String description, Supplier<? extends ScheduledExecutorService> supplier) {
final Tuple2<String, String> metadata = Tuples.of(schedulerType, description);
ScheduledExecutorService result = factory.decorateExecutorService(schedulerType, supplier);
Copy link
Contributor

Choose a reason for hiding this comment

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

Before this PR, the provided wrapper executor service would have been the outer most one (since there could be only one) but here it will be the inner most one. I'm wondering if we should keep this behaviour and call factory.decorateExecutorService at the end.

Copy link
Member Author

Choose a reason for hiding this comment

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

that makes sense, and easy enough to do 👍

@simonbasle simonbasle force-pushed the 1408-schedulersServiceDecorator branch from d219e29 to c0c3aae Compare October 29, 2018 10:21
@simonbasle
Copy link
Member Author

@smaldini can you comment here what you think is missing to merge this PR? So that we can keep track and see if @bsideup can take charge here.

Copy link
Member Author

@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.

LGTM with latest changes from @bsideup, but I can't Approve since I'm the PR author -_- cc @smaldini

@simonbasle simonbasle added the for/fast-track Instructs the bot to fast track (internal) label Nov 20, 2018
Copy link

@reactorbot reactorbot left a comment

Choose a reason for hiding this comment

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

Fast-track requested by @simonbasle

@bsideup bsideup merged commit 568342b into master Nov 20, 2018
@bsideup bsideup deleted the 1408-schedulersServiceDecorator branch November 20, 2018 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for/fast-track Instructs the bot to fast track (internal)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants