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

added start on call and priority for "startables" #37

Closed
wants to merge 2 commits into from

Conversation

jfojtl
Copy link

@jfojtl jfojtl commented Dec 10, 2013

When using Startable Facility, I need to start Components in order, that's why I added an option for prioritization of them. Components with lowest priority number are started at first.

I also need to say, when to start startable components, since we apply installers from multiple assemblies. Anyway, I am sorry that these two funcitonalities are in one commit but I made it all pretty fast since it was pretty easy to add it.

If the pull request gets merged, I am also ready to update the documentation. Thanks for opinions and much appreciated code review.

@kkozmic
Copy link
Contributor

kkozmic commented Dec 30, 2013

Hey,

It's functionality I was thinking about myself. I had a slightly different API in mind though.

var flag = new StartFlag();

container.AddFacility<StartableFacility>(f => f.DeferredStart(flag))
container.Install(FromAssembly.This());

// do some extra work

flag.Signal();

as for the priority... this could be solved by having multiple named tokens, so the components that need to be started first are started when the first flag is signalled, and the rest when the default flag is signalled...

@jfojtl
Copy link
Author

jfojtl commented Jan 2, 2014

Who should be responsible for calling flag.Signal()? With multiple "named tokens", in composition root one would have:

firstsFlag.Signal();
secondsFlag.Signal();
thirdsFlag.Signal();
?

And how would you say which flag starts which startable?

@kkozmic
Copy link
Contributor

kkozmic commented Jan 2, 2014

Who should be responsible for calling flag.Signal()?

whoever manages the StartFlag, which I imagine 99% of the time would be the bootstrapper/composition root.

And how would you say which flag starts which startable?

I'm thinking you'd only have one StartFlag. If you want to have multiple start groups, you'd name them, so the example would become:

flag.Signal("first-group");
flag.Signal("second-group");
flag.Signal(); // the default non-named group.

On the registration side of things, I imagine you'd mark the component with the start group name somehow. Perhaps like:

container.Register(Component.For<SomeClass>()
            .StartInGroup("first-group")
);

What do you think?

@jfojtl
Copy link
Author

jfojtl commented Jan 2, 2014

I like the idea, that you solve the problem with both, priority of start and signal to start "startables" with one API. However, with my use case, I don't usually have "groups" of startables but single startable (each group would contain one startable).

Now I need to have registration by name in installers and then the chain of hierarchy on other place. Really not sure what I like more.

@jfojtl
Copy link
Author

jfojtl commented Jan 2, 2014

The problem with numbers is also refactoring (changing the chain of startables). With group names, It would be definitely easier.

@kkozmic
Copy link
Contributor

kkozmic commented Jan 2, 2014

I don't usually have "groups" of startables but single startable (each group would contain one startable)

Can you elaborate on that a bit more? I haven't come across this scenario yet

@jfojtl
Copy link
Author

jfojtl commented Jan 3, 2014

When dealing with startables, I have a few concrete services, where one
depends on other and that's why I need priority. As a result, I do not have
groups of startables with the same priority but only a concrete services
and between each of these I need this "priority".

Hopefuly, it's understandable. However, it can also be implemented with
your design. What I do not like about my aproach is that priorities are
spread over all of installers and it becomes a mess very easily. With your
aproach, on the other hand, when I add a new group, I need to edit
installer and composition root as well. It goes against my modular
architecture (one module contains all its need to initialize without
changing composition root).
On Jan 2, 2014 11:23 PM, "Krzysztof Koźmic" notifications@github.com
wrote:

I don't usually have "groups" of startables but single startable (each
group would contain one startable)

Can you elaborate on that a bit more? I haven't come across this scenario
yet


Reply to this email directly or view it on GitHubhttps://github.com//pull/37#issuecomment-31490239
.

@kkozmic
Copy link
Contributor

kkozmic commented Jan 3, 2014

Cool, I think we're getting somewhere :)

One thing I'm wondering, is if it woundn't be a good idea to split this into two parts.

One would be implementing the support for StartFlag and explicit signalling. This would be provided out of the box.

The other one, would be building extension points to allow us to easily support the other, advanced scenario, with multiple groups. The actual implementation of that could be provided via another package.

The reasoning for that is, I want Windsor to be slim and provide high impact features out of the box, as well as have simple extension points to allow for less common/more complex scenarios, thus keeping the core small and the basic API simple.

@jfojtl
Copy link
Author

jfojtl commented Jan 3, 2014

I was thinking about new "facility" as well.

However, I'm still not sure about the final solution. Do you want me to create a new pull request where we will keep desinging and here, keep just the explicit signalling?

@kkozmic
Copy link
Contributor

kkozmic commented Jan 3, 2014

sounds good

Krzysztof Kozmic

On Friday, 3 January 2014 at 8:05 pm, jfojtl wrote:

I was thinking about new "facility" as well.
However, I'm still not sure about the final solution. Do you want me to create a new pull request where we will keep desinging and here, keep just the explicit signalling?


Reply to this email directly or view it on GitHub (#37 (comment)).

@kkozmic
Copy link
Contributor

kkozmic commented Feb 10, 2014

Hey,

How's it going? I'm thinking we might want to do another release perhaps within the next month. It would be nice to have the basic functionality for that in it, as we discussed. Do you think you'll have to time to clean up this pull request so we can merge it?

If not that's fine. I'm happy to jump in and implement it myself if you're too busy.

@jfojtl
Copy link
Author

jfojtl commented Feb 13, 2014

Hi,

Sorry, pretty busy last few months. I'll do it during the weekend.

@kkozmic
Copy link
Contributor

kkozmic commented Mar 2, 2014

Hey, I've got some spare time so I may have a stab at implementing it today, unless you've beaten me to it and have it updated and ready somewhere already :)

kkozmic added a commit that referenced this pull request Mar 15, 2014
…s too

Moved all the start control logic to StartFlag, ensuring the important methods are virtual. That way the code can be extended to support any scenario imaginable, including the ones discussed with  @jfojtl (see #37)
@kkozmic
Copy link
Contributor

kkozmic commented Mar 15, 2014

I implemented the functionality as we discussed. Thanks for the feedback. Let me know if you have any comments about the code.

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.

2 participants