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

Allow for a Sirius initialization callback. (Issue #17) #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow for a Sirius initialization callback. (Issue #17) #21

wants to merge 1 commit into from

Conversation

comcast-jonm
Copy link
Member

Adds some messages to the SiriusSupervisor so that you can
send it a message to register interest in an initialization
event and get a message back once that has occurred. Added
an onInitialized() method to SiriusImpl to hook that up to
a callback. This addresses issue #17.

Also defined a new trait/interface Sirius1Dot2 that
extends Sirius. This seems like a good way to track
the additional methods without losing backwards
compatibility with version 1.1.4. In theory, this new
interface would be modifiable up until the next release
(which should be 1.2.0 in this case).

Discussion points:

  • onInitialized takes a java.lang.Runnable as a callback to
    facilitate calling it from Java. Kinda kludgy, since from
    Scala it would just want a lazy => Unit.
  • Does the approach with the additional interface seem ok?

Adds some messages to the SiriusSupervisor so that you can
send it a message to register interest in an initialization
event and get a message back once that has occurred. Added
an onInitialized() method to SiriusImpl to hook that up to
a callback.

Also defined a new trait/interface `Sirius1Dot2` that
extends `Sirius`. This seems like a good way to track
the additional methods without losing backwards
compatibility with version 1.1.4. In theory, this new
interface would be modifiable up until the next release
(which should be 1.2.0 in this case).

Discussion points:
* onInitialized takes a java.lang.Runnable as a callback to
  facilitate calling it from Java. Kinda kludgy, since from
  Scala it would just want a lazy `=> Unit`.
* Does the approach with the additional interface seem ok?
@smuir
Copy link
Contributor

smuir commented Feb 12, 2014

Is the change that introduces Sirius1Dot2 separable from the callback implementation? I think that is a good idea independent of whether the specific implementation of initialisation completion notification via callback is the best one (or at least satisfactory).

@comcast-jonm
Copy link
Member Author

Sure, it is separable if desired. I included it primarily because adding the implementation without extending an interface somewhere doesn't really address the issue completely, and because the interface extension doesn't really stand by itself without associated implementation that needs to be exposed.

@mattinger
Copy link
Member

I like the extra trait the way it's implemented. The only question I really have is do we want to actual call to the runnables to be synchronous as they are now, or would it be better to send the callbacks asynchronously? I'm half thinking we could use rxjava and instead add a method to get an observable which can be used to hook into the boot process

def bootObservable : Observable

Then let the client subscribe as they want

val subscription = sirius.bootObservable.subscribe { ... }

And this way the client decides whether to be notified synchronously or asynchrounously (if i interperate the rxjava api correct) by optionally providing a scheduler instance as well

val subscription = sirius.bootObservable.subscribe({ ... }, new ExecutorScheduler(executorService))

val subscription = sirius.bootObservable.subscribe({ ... }, new CurrentThreadScheduler)

@smuir
Copy link
Contributor

smuir commented Feb 13, 2014

Jon - my thought about separation was that the addition of a new interface as the basis for adding new features is very clean and can probably stand alone, right? Even if it is just an empty interface initially, i.e., without the new callback.

@smuir
Copy link
Contributor

smuir commented Feb 13, 2014

As for the specific method for notifying the Sirius user that initialisation has completed, my original motivation was to simplify app development, hence my suggestion of something as straightforward as a blocking wait() call. The other possibilities are undoubtedly nice, and much more flexible, but they still impose complexity on the app developer if all that the developer wants to do is wait for completion of initialisation. Maybe most 21st century developers are more sophisticated than I am though.

@comcast-jonm
Copy link
Member Author

@mattinger , @smuir : These are all fine alternatives for how to implement the callback. The Observable is a great approach but would add a dependency on Rx, something I think is kind of cheeky when the core of the library doesn't really use it. If an application is already using Rx, it's relatively easily for the application to wrap the Runnable in whatever they want.

The blocking call is a possibility I hadn't considered; perhaps I'll add another method blockForInitialization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants