Schedulers Interface (Merging and Adding to Pull Request 229) #235

benjchristensen · 2013-04-17T05:44:35Z

Merging and adding to pull request #229 from @jmhofer which adds functionality discussed in the Scheduler issue #19

Work done in #229 added the following methods to Scheduler:

Subscription schedule(T state, Func2<Scheduler, T, Subscription> action, long delayTime, TimeUnit unit)
Subscription schedule(T state, Func2<Scheduler, T, Subscription> action)

These are in fact the primary methods from RxNet (http://msdn.microsoft.com/en-us/library/hh211963(v=vs.103).aspx) and the others are just helper overloads.

It seems beneficial to use these 2 methods for actual implementation logic while all other methods are just decorating and forwarding from AbstractScheduler to these methods.

I propose these changes on top of pull request #229 to achieve this.

I also added unit tests that @mairbek created (#229 (comment)).

All unit tests are passing … but we don't yet have enough unit test coverage so I won't be surprised if bugs are found.

ReactiveX#19.

…l-229-merge

- also a utility method for creating a Subscription around a Future

Work done in ReactiveX#229 added the following methods: - Subscription schedule(T state, Func2<Scheduler, T, Subscription> action, long delayTime, TimeUnit unit)} - Subscription schedule(T state, Func2<Scheduler, T, Subscription> action)} These are in fact the primary methods from RxNet (http://msdn.microsoft.com/en-us/library/hh211963(v=vs.103).aspx) and the others are just helper overloads. It seems it is better to set the precedent to use these 2 methods for actual implementation logic while all other methods are just decorating and forwarding from AbstractScheduler to these methods. I have updated the various implementations to achieve this. Unit tests are passing … but we don't have enough unit test coverage so I won't be surprised if bugs are found.

@mairbek

These tests came from @mairbek at ReactiveX#229 (comment)

benjchristensen · 2013-04-17T05:48:54Z

rxjava-core/src/main/java/rx/concurrency/AbstractScheduler.java


-/* package */abstract class AbstractScheduler implements Scheduler {


I made this public because as currently designed anyone implementing a Scheduler will almost certainly want to use this.

Copy/pasting from #229 (comment):

The .Net implementation is able to use extension methods to make the design much more elegant where only the main 3 methods are part of the Scheduler interface and the rest come along for the ride.

It means we end up with a Scheduler/AbstractScheduler Interface/Abstract pairing to make this work.

Should we just make Scheduler an Abstract? I'm very tempted to do so because of the following problems:

adding methods to Scheduler will be breaking changes requiring major version increments

people implementing Schedulers will basically always have to also extend AbstractScheduler or copy/paste all of those method overloads

Or should be remove all but the main 3 methods from Scheduler and put all the overloads as utility functions on the Schedulers class instead?

The precedent for using abstract (or concrete) classes instead of interfaces (which .Net then augments with extension methods) is already done - Observable is a concrete class instead of interface for this very reason.

All plugins are done as abstracts instead of interfaces for this reason as well.

Thoughts?

I don't see any problems with all the overloaded methods here exactly because they are all pre-implemented in AbstractScheduler. I'm not sure whether there's any advantage in having Scheduler as interface separate from the AbstractScheduler class. It might be more confusing than useful. - I'm very used to traits (in Scala) already, though...

I thought it would be more flexible to have an interface but it turns out to be redundant. I agree that abstract class would be a better choice.

I'll merge the two then into a single abstract Scheduler class.

Too bad it will be a very long time before we can use Java8 as the compile-time target...

cloudbees-pull-request-builder · 2013-04-17T05:49:27Z

RxJava-pull-requests #94 SUCCESS
This pull request looks good

benjchristensen · 2013-04-17T05:50:02Z

rxjava-core/src/main/java/rx/concurrency/AbstractScheduler.java

+
+    @Override
+    public Subscription schedule(final Func1<Scheduler, Subscription> action, long delayTime, TimeUnit unit) {
+        return schedule(null, new Func2<Scheduler, Void, Subscription>() {


I'm curious if passing null could cause problems but I can't think of a valid "Optional" type that makes sense here.

benjchristensen · 2013-04-17T06:01:41Z

@jmhofer @mairbek (and any others of course) I have added various inline comments and questions about the Scheduler code and would appreciate your thoughts (even if it's stating agreement but especially if you disagree) to determine if our approaches to these various things are correct.

Thank you both for your involvement in this so far and continuing to do so!

benjchristensen · 2013-04-17T06:35:40Z

rxjava-core/src/main/java/rx/Scheduler.java

+     *            Time unit of the delay time.
+     * @return a subscription to be able to unsubscribe from action.
+     */
+    <T> Subscription schedule(T state, Func2<Scheduler, T, Subscription> action, long delayTime, TimeUnit unit);


Should long delayTime, TimeUnit unit become a TimeSpan class that holds them together like Rx.Net does or leave it the "Java way" as ScheduledExecutorService does (http://docs.oracle.com/javase/6/docs/api/index.html?java/util/concurrent/ScheduledExecutorService.html) with the two kept separate?

The TimeSpan class is a system class in .Net (http://msdn.microsoft.com/en-us/library/system.timespan(v=vs.103).aspx) whereas Java doesn't have anything like that - it's always just a long + TimeUnit

If we were to add this class should it be in rx.concurrency or rx.util? I lean towards rx.util.

This discussion originates from #228 (comment)

I'd strongly prefer a TimeSpan class, because this is what we mean semantically. delayTime and TimeUnit make no sense to me as standalone parameters. This can lead to bugs, for example when people copy over delayTime somewhere but forget to copy the time units, too.

I don't like it either, but I think it is reasonable to leave it the way it works now:

People who use Java will find it consistent with familiar JDK APIs.

Java standard API doesn't have similar abstractions. At least in version 6.

Introducing date-time abstractions in public API will add a bit of "noise" to the library, since it is not actually the Rx stuff.

I tend to agree with @mairbek since this is on the JVM and the pattern established by it is to have delayTime+TimeUnit ... though this is not a strong opinion on my part. I could go either way.

In spite of my preference I find myself kind of reluctantly agreeing with @mairbek too, due to 3.: date-time abstractions should probably be kept separate from Rx.

Then I will leave this as is.

- using abstract class for Scheduler for same reason Observable is concrete - discussed and decided upon at ReactiveX#235

benjchristensen · 2013-04-17T22:03:49Z

@mairbek and @jmhofer I think this pull request is ready to merge though I'm not yet ready to release. I'd like to leave some more time to review, consider and fix things.

Agree? Disagree?

benjchristensen · 2013-04-17T22:05:07Z

rxjava-core/src/main/java/rx/Scheduler.java

     */
-    long now();
+    public long now() {
+        return System.currentTimeMillis();


Note that this is in milliseconds since it must be an absolute time that can be used to create a Date and getNanos() does not provide that.

This has implications for the SleepingAction which will need to change: #236

See how line 87 above is using now() to get milliseconds and interact with Date.

- added method: schedule(T state, Func2<Scheduler, T, Subscription> action, Date dueTime)

cloudbees-pull-request-builder · 2013-04-17T22:09:13Z

RxJava-pull-requests #96 SUCCESS
This pull request looks good

jmhofer · 2013-04-18T07:25:29Z

+1 from me to merging, but not releasing yet.

Schedulers Interface (Merging and Adding to Pull Request 229)

- using abstract class for Scheduler for same reason Observable is concrete - discussed and decided upon at ReactiveX#235

…-229-merge Schedulers Interface (Merging and Adding to Pull Request 229)

jmhofer and others added 5 commits April 9, 2013 15:55

Trying to extend the Scheduler interface according to the comments at

0950c46

ReactiveX#19.

Merge branch 'schedulers' of git://github.com/jmhofer/RxJava into pul…

c704c5c

…l-229-merge

Add CompositeSubscription

2b0b684

- also a utility method for creating a Subscription around a Future

Add unit tests for recursive scheduler usage

770ee73

These tests came from @mairbek at ReactiveX#229 (comment)

benjchristensen mentioned this pull request Apr 17, 2013

Trying to extend the Scheduler interface according to the comments at #229

Merged

benjchristensen reviewed Apr 17, 2013
View reviewed changes

This was referenced Apr 17, 2013

Implement ReplaySubject with infinite history #218

Merged

Interval #228

Merged

benjchristensen reviewed Apr 17, 2013
View reviewed changes

Merge Scheduler/AbstractScheduler

7a8b681

- using abstract class for Scheduler for same reason Observable is concrete - discussed and decided upon at ReactiveX#235

benjchristensen reviewed Apr 17, 2013
View reviewed changes

Implement Scheduler method with dueTime

d2a3f29

- added method: schedule(T state, Func2<Scheduler, T, Subscription> action, Date dueTime)

benjchristensen added a commit that referenced this pull request Apr 18, 2013

Merge pull request #235 from benjchristensen/schedulers-pull-229-merge

b66557c

Schedulers Interface (Merging and Adding to Pull Request 229)

benjchristensen merged commit b66557c into ReactiveX:master Apr 18, 2013

benjchristensen deleted the schedulers-pull-229-merge branch April 18, 2013 16:11

rickbw pushed a commit to rickbw/RxJava that referenced this pull request Jan 9, 2014

Merge Scheduler/AbstractScheduler

0b3110d

- using abstract class for Scheduler for same reason Observable is concrete - discussed and decided upon at ReactiveX#235

rickbw pushed a commit to rickbw/RxJava that referenced this pull request Jan 9, 2014

Merge pull request ReactiveX#235 from benjchristensen/schedulers-pull…

a30d270

…-229-merge Schedulers Interface (Merging and Adding to Pull Request 229)

jihoonson pushed a commit to jihoonson/RxJava that referenced this pull request Mar 6, 2020

Issue ReactiveX#235: TimeLimiter as rx transformer (ReactiveX#624)

b8d39d8

jihoonson pushed a commit to jihoonson/RxJava that referenced this pull request Mar 6, 2020

Issue ReactiveX#235: TimeLimiter as a Reactor operator (ReactiveX#636)

3267aed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Schedulers Interface (Merging and Adding to Pull Request 229) #235

Schedulers Interface (Merging and Adding to Pull Request 229) #235

benjchristensen commented Apr 17, 2013

benjchristensen Apr 17, 2013

jmhofer Apr 17, 2013

mairbek Apr 17, 2013

benjchristensen Apr 17, 2013

cloudbees-pull-request-builder commented Apr 17, 2013

benjchristensen Apr 17, 2013

benjchristensen commented Apr 17, 2013

benjchristensen Apr 17, 2013

jmhofer Apr 17, 2013

mairbek Apr 17, 2013

benjchristensen Apr 17, 2013

jmhofer Apr 17, 2013

benjchristensen Apr 17, 2013

benjchristensen commented Apr 17, 2013

benjchristensen Apr 17, 2013

benjchristensen Apr 17, 2013

cloudbees-pull-request-builder commented Apr 17, 2013

jmhofer commented Apr 18, 2013


		/* package */abstract class AbstractScheduler implements Scheduler {

Schedulers Interface (Merging and Adding to Pull Request 229) #235

Schedulers Interface (Merging and Adding to Pull Request 229) #235

Conversation

benjchristensen commented Apr 17, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudbees-pull-request-builder commented Apr 17, 2013

Choose a reason for hiding this comment

benjchristensen commented Apr 17, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjchristensen commented Apr 17, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudbees-pull-request-builder commented Apr 17, 2013

jmhofer commented Apr 18, 2013