Start to split AppExecutor, call events on restart of resources#7206
Merged
Start to split AppExecutor, call events on restart of resources#7206
Conversation
3 tasks
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Member
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
davidfowl
reviewed
Jan 23, 2025
|
|
||
| namespace Aspire.Hosting.Dcp; | ||
|
|
||
| internal record ResourceStatus(string? State, DateTime? StartupTimestamp, DateTime? FinishedTimestamp); |
Contributor
There was a problem hiding this comment.
Why didn't you use an interface here? Do we have multiple subscribers?
Member
Author
There was a problem hiding this comment.
There are never multiple subscribers for these events. Just ApplicationOrchestrator uses this to listen to changes from DCP.
Are you asking about why not an interface for DcpExecutorEvents? There is only one implementation. The type could disappear and it becomes part of DcpExecutor/IDcpExecutor, but having it separated feels more usable in tests.
Contributor
There was a problem hiding this comment.
I’m asking why isn’t the dcp executor just calling interface methods to raise the events
166d3d6 to
bb806ac
Compare
Open
3 tasks
01fc6e7 to
5cd3d7a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR is a start towards making
ApplicationExecutorless of a monster. The newDcpExecutoris about 400 lines smaller.ApplicationOrchestrator. The orchestrator starts the DCP executor and subscribes to events from it. The orchestrator raises Aspire lifecycle events and publishes resource notifications. It handles managing parent/child notifications.ApplicationExecutortoDcpExecutor.IDcpExecutor. AllowsApplicationOrchestratorto be tested independently.DcpExecutorEvents. Use by DCP executor to publish events and the orchestrator to subscribe. Copies whatDistributedApplicationEventingdoes but much simplified.ResourceSnapshotPublisher. Moves building resource snapshots from DCP resources out into a seperate file.DcpHost. This splits the logic related to starting DCP process into its own file.DcpHostServiceintoOrchestratorHostService. The orchestrator host service starts the DCP process usingDcpHostthen starts the orchestrator.StartResourceAsyncto call the OnResourceStarting event, which then triggers Aspire lifecycle events for the resource (BeforeResourceStartedEvent, waiting for health checks, etc)StartResourceAsyncto set a resource toFailedToStartstate if there is an exeception.TODO:
StartResourceAsyncimprovementsFixes #7011
Checklist
<remarks />and<code />elements on your triple slash comments?breaking-changetemplate):doc-ideatemplate):