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.

Sign up for GitHub

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

Jump to bottom

introduce ContextAware interface, store tracking on the context #1492

Merged
merged 10 commits into from
Jan 28, 2019
Merged

introduce ContextAware interface, store tracking on the context #1492

merged 10 commits into from
Jan 28, 2019

Conversation

bsideup
Copy link
Contributor

@bsideup bsideup commented Jan 25, 2019 •
edited

  1. Introduced schedule*(ContextRunnable, ...) overloads
  2. “find usages” on the (Runnable) one
  3. replaced with ContextRunnable
  4. repeated until there are no usages of Runnable except in CachedScheduler and tests

@bsideup
Copy link
Contributor Author

bsideup commented Jan 25, 2019

Hi @akarnokd,

I assigned you as a reviewer as well in case you have time to take a look ☺️

simonbasle
simonbasle requested changes Jan 25, 2019
View reviewed changes
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.

interesting change, a few things to improve but I like the idea of being able to access the Context from scheduled tasks. @smaldini there are some implications on gc pressure for a few instances where the scheduled Runnable was a method reference and cannot be the instance itself (because it already implements Runnable for another task)

reactor-core/src/main/java/reactor/core/publisher/FluxSubscribeOnValue.java Outdated Show resolved Hide resolved
reactor-core/src/main/java/reactor/core/publisher/FluxWindowTimeout.java Outdated Show resolved Hide resolved
reactor-core/src/main/java/reactor/core/publisher/FluxWindowTimeout.java Outdated Show resolved Hide resolved
reactor-core/src/main/java/reactor/core/publisher/MonoCacheTime.java Outdated Show resolved Hide resolved
reactor-core/src/main/java/reactor/core/publisher/MonoCacheTime.java Show resolved Hide resolved
reactor-core/src/main/java/reactor/core/publisher/FluxSubscribeOnCallable.java Show resolved Hide resolved
reactor-core/src/main/java/reactor/core/scheduler/Scheduler.java Show resolved Hide resolved
reactor-core/src/main/java/reactor/core/scheduler/Scheduler.java Show resolved Hide resolved
reactor-core/src/main/java/reactor/core/scheduler/Scheduler.java Show resolved Hide resolved
reactor-core/src/main/java/reactor/core/scheduler/Scheduler.java Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Jan 25, 2019 •
edited

Codecov Report

Merging #1492 into tracing will increase coverage by <.01%.
The diff coverage is 58.88%.

Impacted file tree graph

@@              Coverage Diff              @@
##             tracing    #1492      +/-   ##
=============================================
+ Coverage      83.85%   83.85%   +<.01%     
+ Complexity      3941     3918      -23     
=============================================
  Files            363      363              
  Lines          30122    29968     -154     
  Branches        5585     5526      -59     
=============================================
- Hits           25259    25131     -128     
+ Misses          3214     3194      -20     
+ Partials        1649     1643       -6
Impacted Files Coverage Δ Complexity Δ
...n/java/reactor/core/scheduler/SingleScheduler.java 76.59% <ø> (ø) 17 <0> (ø) ⬇️
...ain/java/reactor/core/publisher/FluxPublishOn.java 86.59% <ø> (ø) 6 <0> (ø) ⬇️
...ore/publisher/FluxOnBackpressureBufferTimeout.java 91.17% <ø> (ø) 4 <0> (ø) ⬇️
...ava/reactor/core/scheduler/ImmediateScheduler.java 76.19% <ø> (ø) 6 <0> (ø) ⬇️
...java/reactor/core/scheduler/ExecutorScheduler.java 79.02% <ø> (-0.7%) 12 <0> (ø)
...ain/java/reactor/core/publisher/MonoPublishOn.java 85.71% <ø> (ø) 3 <0> (ø) ⬇️
...java/reactor/core/scheduler/ParallelScheduler.java 78.26% <ø> (ø) 24 <0> (ø) ⬇️
...actor/core/scheduler/DelegateServiceScheduler.java 60.97% <ø> (ø) 11 <0> (ø) ⬇️
.../reactor/core/scheduler/ExecutorServiceWorker.java 71.42% <ø> (ø) 7 <0> (ø) ⬇️
.../reactor/core/scheduler/SingleWorkerScheduler.java 70% <ø> (ø) 10 <0> (ø) ⬇️
... and 35 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 d53b85a...d54ed29. Read the comment docs.

simonbasle
simonbasle reviewed Jan 28, 2019
View reviewed changes
reactor-core/src/main/java/reactor/core/scheduler/Scheduler.java Outdated
@@ -47,9 +47,28 @@
*
* @return the {@link Disposable} instance that let's one cancel this particular task.
* If the {@link Scheduler} has been shut down, throw a {@link RejectedExecutionException}.
*
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: remove the newlines between @ javadoc tags (keep one between last line of body and first tag)

Copy link
Member

Choose a reason for hiding this comment

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

(+ a few other occurrences in the javadocs below)

simonbasle
simonbasle approved these changes Jan 28, 2019
View reviewed changes
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.

a tiny nitpick on the javadoc formatting (see comment), but otherwise looks good 👍

simonbasle
simonbasle reviewed Jan 28, 2019
View reviewed changes
reactor-test/src/main/java/reactor/test/scheduler/VirtualTimeScheduler.java Outdated
@@ -252,6 +254,7 @@ public Disposable schedule(Runnable task) {
}

@Override
@SuppressWarnings("deprecation")
Copy link
Member

Choose a reason for hiding this comment

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

since VirtualTimeScheduler is public API, it should not suppress the warning. (I think it is the only public class that is suppressing the warning, but please double check)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch 👍 Removed the suppression

@bsideup bsideup merged commit d11fa50 into tracing Jan 28, 2019
@reactor reactor deleted a comment from reactorbot Jan 28, 2019
@bsideup bsideup mentioned this pull request Jan 28, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonbasle simonbasle simonbasle approved these changes

@smaldini smaldini Awaiting requested review from smaldini

@akarnokd akarnokd Awaiting requested review from akarnokd

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bsideup @codecov-io @simonbasle @reactorbot