Add startup tasks to replace deprecated bootstrap providers#4026
Conversation
There was a problem hiding this comment.
Is this a suitable name? Is the class suitably named? I'm open to suggestions.
There was a problem hiding this comment.
Maybe Run or Execute? It's not like this code signs up for a notification about silo startup, more like it simply wants to get executed when the environment is ready.
There was a problem hiding this comment.
Maybe Run or Execute? It's not like this code signs up for a notification about silo startup, more like it simply wants to get executed when the environment is ready.
|
Execute sounds fine to me |
|
This is fine, but it seems to be more complexity than is necessary with limited flexibility. Specifics The scheduler problem needs be solved for all providers, and we've discussed how to address that, so there is no reason for the scheduling issue to be addressed in this change. This is going against the planned work and will just introduce more work, as we'll need to remove it. The interface has only one method, so there is no need for the interface, as we can simply provide a startup action (or delegate) (as is provided as an option in this PR). The StartupTask, by calling all of the startup tasks, forces all the startup actions to be run in the same stage and executes them sequentially rather than in parallel. The StartupTask needs only take an action and a stage to execute it in. For each configured startup action we'd register a StartupTask as a lifecycle participant. Public surface Participant This would be less code, simpler and more flexible (IMHO). |
|
The reason for an interface over just an delegate is that a delegate is useless if it can't actually interact with the silo (eg, needs a grain factory), so an interface allows that to be injected. Of course, both models are equivalent in a sense. What are the use cases for allowing different stages? Maybe we can add support for that when they arise instead. Regarding scheduling, how do you think it should look? The whole lifecycle cannot be handled by OrleansTaskScheduler, since that only becomes available during the lifecycle. Should we pull it out? Should the lifecycle.Subscribe method take an optional scheduling context? |
I like the simplicity and discoverability of the interface model. More complex extensions like providers need deeper integration with the silo lifecycle. But for startup tasks (replacement for bootstrap providers) I think we'd better stay simple. |
|
Interface vs action Stages
Services exist now with both cases. Scheduling
While it's not clear what the best solution is, I'm convinced solving it exclusively for startup tasks is probably not what we want to do.. |
|
Usually these services don't care about actually being scheduled on the OrleansTaskScheduler, they just want to be able to interact with the runtime/grains. This is entirely solved via silo-local client.
I'm ok with that.
How might this look?
We had an implementation of that in a PR (#3290 (comment)) but it was reverted before merging. I wasn't sold on that approach. Regarding configurable stages, I'm happy to defer to Sergey. Personally I think less options is generally better, as long as there's an escape hatch (i.e, we let users manually implement ILifecycleParticipant for anything nuanced.) |
I don't think StartupTask should have the ability to subscribe to a certain stage. We got
I think we should have this. It provides a way to access Orleans context from |
|
+1 for @xiazen's:
The more complex cases like
will require a full implementation of |
|
There's still the issue that this PR does not address the general problem of calling grains from ILifecycleParticipants. I agree that that's an issue. The questions are:
In my opinion, silo-local client is the best solution, but that wont be a part of 2.0. I'm happy to leave it for now and try to solve it in 2.1 using that approach. I'm wary of having many unrelated services competing for a time slice on single threaded scheduler when they shouldn't need to, so I'm not encouraging the approach from the PR I mentioned above. |
I think not as part of this PR. A separate PR would be easier, even if it ends up touching the same code as this one. |
|
I don't think we should solve it in this PR. As long as refactoring the code doesn't touch public interfaces, I think we can even solve that after rc. But I do think we should solve it before 2.0 . Since in 1.5, providers can access Orleans context in their init logic. For example, in |
|
almost feels to me that the other pr should go first, and then this pr. Since this PR really should just be using the infra built on the other PR. But I'm fine with making compromise due to our shipping deadline on rc. |
I don't see this as a replacement of bootstrap provider, as much as syntactic sugar over the lifecycle pattern for simple startup tasks. If it is a replacement for the bootstrap providers, the legacy providers, at this time, do allow users to specify the stage in which they start, so without the ability to set the stage, this is a regression. As syntactic sugar, allowing one to (optionally) set the stage (as it was introduced with an optional parameter) affords much utility, with almost no extra complexity. The gain, imo, far outweighs the cost. As mentioned, we have services that have needed the ability to set startup tasks at various stages, and such requests are one of the reasons we've moved to this model in the first place. I'm unclear why we're adding syntactic sugar which limits capabilities unnecessarily. IMO, for this, something like the below is simple, more flexable, and all we really need. |
1250a57 to
2489ba6
Compare
2489ba6 to
98e8a2b
Compare
98e8a2b to
5ba13ef
Compare
|
@jason-bragg does the latest revision seem closer to what you envisioned? |
| RegisterSystemTarget(fallbackScheduler); | ||
|
|
||
| // SystemTarget for startup tasks | ||
| var startupTaskTarget = Services.GetRequiredService<StartupTaskSystemTarget>(); |
There was a problem hiding this comment.
Not sure what we need to setup another task scheduler, as we already have and use fallback for this. It's not a bad thing, just not convinced it's necessary.
There was a problem hiding this comment.
It's not necessary, but during our meeting it was mentioned, so I added a new special-purpose one. We can always axe it, especially when we fix scheduling of lifecycle participants more broadly
|
LG2M, though it think much of this will go away when we fix the general lifecycle scheduling issue. |
Resolves #4014
Developers can register an implementation of
IStartupTaskand perform operations such as calling grains from within theOnStarted()method.Added a new
SystemTargetfor the purpose of scheduling startup tasks. I didn't extend scheduling to all lifecycle participants because the scheduler itself is only started at a certain silo lifecycle stage and therefore wont pump tasks before then. If this is an issue, maybe someone has an idea for how to solve it?See the tests at the bottom for usage.