Add simplified API for wrapping scheduled tasks. #1546

bsideup · 2019-02-26T21:22:15Z

Schedulers.onScheduleHook(String, Function<Runnable,Runnable>) is a new
API for wrapping the tasks submitted to Reactor's schedulers.

codecov-io · 2019-02-27T07:26:32Z

Codecov Report

Merging #1546 into master will increase coverage by 0.13%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #1546      +/-   ##
============================================
+ Coverage      84.2%   84.33%   +0.13%     
- Complexity     3907     3918      +11     
============================================
  Files           359      359              
  Lines         29920    29958      +38     
  Branches       5566     5574       +8     
============================================
+ Hits          25194    25265      +71     
+ Misses         3096     3075      -21     
+ Partials       1630     1618      -12

Impacted Files	Coverage Δ	Complexity Δ
...re/src/main/java/reactor/core/publisher/Hooks.java	`96.31% <ø> (ø)`	`41 <0> (ø)`	⬇️
...c/main/java/reactor/core/scheduler/Schedulers.java	`85.71% <100%> (+2.62%)`	`83 <23> (+10)`	⬆️
...rc/main/java/reactor/core/publisher/Operators.java	`79.49% <0%> (+0.14%)`	`123% <0%> (ø)`	⬇️
...ain/java/reactor/core/publisher/FluxPublishOn.java	`86.8% <0%> (+0.2%)`	`6% <0%> (ø)`	⬇️
...ain/java/reactor/core/publisher/FluxConcatMap.java	`90.42% <0%> (+0.28%)`	`7% <0%> (ø)`	⬇️
...va/reactor/core/publisher/FluxMergeSequential.java	`82.75% <0%> (+0.38%)`	`6% <0%> (ø)`	⬇️
...eactor/core/publisher/ParallelMergeSequential.java	`80.31% <0%> (+0.51%)`	`7% <0%> (ø)`	⬇️
...in/java/reactor/core/publisher/TopicProcessor.java	`74.85% <0%> (+0.59%)`	`16% <0%> (ø)`	⬇️
...c/main/java/reactor/core/publisher/FluxReplay.java	`84.78% <0%> (+0.62%)`	`25% <0%> (ø)`	⬇️
...java/reactor/core/scheduler/ExecutorScheduler.java	`79.86% <0%> (+0.69%)`	`12% <0%> (ø)`	⬇️
... and 8 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 e8c8a1c...9a4cb12. Read the comment docs.

reactor-core/src/main/java/reactor/core/publisher/HookableScheduledExecutorService.java

simonbasle · 2019-02-27T16:25:52Z

reactor-core/src/main/java/reactor/core/publisher/HookableScheduledExecutorService.java

+	public <V> ScheduledFuture<V> schedule(Callable<V> callable,
+			long delay,
+			TimeUnit unit) {
+		return (ScheduledFuture<V>) delegate.schedule(wrap(callable), delay, unit); //FIXME


@bsideup @smaldini I can see why the new interface was proposed to map both Runnable and Callable. Maybe instead of hard casting we should throw UOE, saying that reactor internals are not supposed to call ScheduledExecutorService Callable-based methods?

And probably hunt down every place where we have access to an executor to introduce a comment that such executors' Callable-based methods are off limits?

Since we leak the ScheduledExecutorService to our public API, these UOE make me feel uneasy to be honest. Did you come to some agreement with @smaldini about this?

simonbasle · 2019-02-27T16:26:06Z

reactor-core/src/main/java/reactor/core/publisher/HookableScheduledExecutorService.java

+
+	@Override
+	public <T> Future<T> submit(Callable<T> task) {
+		return (Future<T>) delegate.submit(wrap(task)); //FIXME


same here as FIXME above

simonbasle · 2019-02-27T16:27:06Z

reactor-core/src/main/java/reactor/core/publisher/Hooks.java

+	 * @see #resetOnSchedule(String)
+	 * @see #resetOnSchedule()
+	 */
+	public static void onSchedule(String key, Function<Runnable, Runnable> decorator) {


@bsideup I aligned the naming of the method and its behavior with other hooks. Main change is that it is now "add or replace" (put). Unless there is a compelling reason for this particular hook to be putIfAbsent unlike the others?

simonbasle · 2019-02-27T16:28:45Z

reactor-core/src/main/java/reactor/core/publisher/Hooks.java

+	 * This key is used to add the {@link #onSchedule(String, Function)} composite hook as
+	 * a {@link Schedulers#addExecutorServiceDecorator(String, BiFunction) executorService decorator}.
+	 */
+	private static final String EXECUTOR_DECORATOR_ONSCHEDULE = "reactor.onSchedule";


@bsideup I changed the constant name a bit and made it private because other constants at this level are used for storing a hook override in the Context (hence the .local suffix). This is different though: a key for the Schedulers own "hook" (the executor decorator).

simonbasle · 2019-02-27T16:32:34Z

reactor-core/src/main/java/reactor/core/scheduler/InstantPeriodicWorkerTask.java

@@ -70,7 +70,7 @@ public Void call() {
 		try {
 			try {
 				task.run();
-				setRest(executor.submit(this));
+				setRest(executor.submit((Callable<Void>) this));


should this cast to Runnable instead? (see discussion in the HookableScheduledExecutorService)

Runnable is the only type that is guaranteed to be maintained by the Function<Runnable, Runnable>... Callable is used internally mostly for short-circuiting subscription IIRC, but I don't think they've been actively used for tasks scheduling purposes so far.

Having this new hook kind of enforces us to limit our use of ExecutorService to Runnable-based methods... cc @smaldini

I did it to align with the rest of tasks. AFAIR the problem is that ScheduledThreadPoolExecutor wraps Runnable into Callable when submitted:
https://github.com/frohoff/jdk8u-jdk/blob/da0da73ab82ed714dc5be94acd2f0d00fbdfe2e9/src/share/classes/java/util/concurrent/FutureTask.java#L152

reactor-core/src/main/java/reactor/core/scheduler/Schedulers.java

changed the implementation

`Hooks.addOnScheduleDecorator(String, ScheduledTaskDecorator)` is a new API for wrapping the tasks submitted to Reactor's schedulers. It's implemented as a "sugar" for `addExecutorServiceDecorator`, but instead of registering a decorator per hook, we register a single decorator and iterate over the registered `ScheduledTaskDecorator`s.

- NotNull (from jetbrains) is not necessary. superseded by NonNullApi (from reactor) at package level anyway. - removed unused throws clauses for methods that throw UOE anyway - Reworked the hook naming and javadoc to reflect what we have with other transformative keyed hooks (onSchedule, resetOnSchedule(String) and resetOnSchedule()). onSchedule adding is now replacing decorators if a key exists.

`Schedulers.onScheduleHook(String, Function<Runnable,Runnable>)` is a new API for wrapping the tasks submitted to Reactor's schedulers. The hooks intercepts `Runnable`s before they passed to the executor.

bsideup added the type/enhancement A general enhancement label Feb 26, 2019

bsideup added this to the 3.3.0.RELEASE milestone Feb 26, 2019

bsideup requested review from smaldini and simonbasle February 26, 2019 21:22

simonbasle reviewed Feb 27, 2019

View reviewed changes

reactor-core/src/main/java/reactor/core/publisher/HookableScheduledExecutorService.java Outdated Show resolved Hide resolved

simonbasle previously requested changes Feb 27, 2019

View reviewed changes

smaldini approved these changes Mar 21, 2019

View reviewed changes

bsideup requested a review from simonbasle March 22, 2019 12:17

simonbasle force-pushed the on_schedule_decorator branch from 7c34464 to e2c9985 Compare March 22, 2019 15:31

simonbasle approved these changes Mar 22, 2019

View reviewed changes

bsideup requested review from smaldini and simonbasle March 22, 2019 16:56

simonbasle approved these changes Mar 27, 2019

View reviewed changes

bsideup and others added 8 commits March 27, 2019 15:11

fix tests, add onScheduleHook pre-computed ScheduledTaskDecorator

c7d73a7

replace ScheduledTaskDecorator with Function<Runnable, Runnable>

8974afb

fix javadoc in Schedulers following Hooks.onSchedule renaming

5f1a562

Re-implement as a pre-schedule decorator

de62426

default value for onScheduleHook

b30fcb2

Move onScheduleHook to Schedulers

9a4cb12

simonbasle force-pushed the on_schedule_decorator branch from 46f7a7a to 9a4cb12 Compare March 27, 2019 14:11

bsideup merged commit 98bda90 into master Mar 28, 2019

bsideup deleted the on_schedule_decorator branch March 28, 2019 20:08

bsideup mentioned this pull request Apr 9, 2019

Basic tracking support #1481

Closed

simonbasle modified the milestones: 3.3.0.RELEASE, 3.3.0.M1 Apr 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add simplified API for wrapping scheduled tasks. #1546

Add simplified API for wrapping scheduled tasks. #1546

bsideup commented Feb 26, 2019 •

edited

codecov-io commented Feb 27, 2019 •

edited

simonbasle Feb 27, 2019

bsideup Mar 7, 2019

simonbasle Feb 27, 2019

simonbasle Feb 27, 2019

simonbasle Feb 27, 2019

simonbasle Feb 27, 2019

bsideup Mar 7, 2019

Add simplified API for wrapping scheduled tasks. #1546

Add simplified API for wrapping scheduled tasks. #1546

Conversation

bsideup commented Feb 26, 2019 • edited

codecov-io commented Feb 27, 2019 • edited

Codecov Report

simonbasle Feb 27, 2019

Choose a reason for hiding this comment

bsideup Mar 7, 2019

Choose a reason for hiding this comment

simonbasle Feb 27, 2019

Choose a reason for hiding this comment

simonbasle Feb 27, 2019

Choose a reason for hiding this comment

simonbasle Feb 27, 2019

Choose a reason for hiding this comment

simonbasle Feb 27, 2019

Choose a reason for hiding this comment

bsideup Mar 7, 2019

Choose a reason for hiding this comment

bsideup commented Feb 26, 2019 •

edited

codecov-io commented Feb 27, 2019 •

edited