Implemented distributed cache refresh for workflow definitions #5095
Conversation
| @@ -24,6 +24,7 @@ | |||
| <ProjectReference Include="..\..\modules\Elsa.Quartz\Elsa.Quartz.csproj"/> | |||
| <ProjectReference Include="..\..\modules\Elsa.Webhooks\Elsa.Webhooks.csproj"/> | |||
| <ProjectReference Include="..\..\modules\Elsa.Workflows.Api\Elsa.Workflows.Api.csproj"/> | |||
| <ProjectReference Include="..\..\modules\Elsa.Workflows.Management.MassTransit\Elsa.Workflows.Management.MassTransit.csproj" /> | |||
There was a problem hiding this comment.
Although you're not wrong to add a separate module for the MT implementation of the new dispatcher service, it is preferable to add it as an additional feature to the existing Elsa.MassTransit module. This maintains the ability to separately use the MassTransit implementation of the cancellation dispatcher, without the need for yet another package - which we would have to add for other implementations, like RabbitMQ, Azure Service Bus, Kafka, etc. if we followed this pattern.
| /// <summary> | ||
| /// Dispatches workflow definition related notifications to refresh the activity registry. | ||
| /// </summary> | ||
| public interface IWorkflowDefinitionDispatcher |
There was a problem hiding this comment.
A better name might be IDistributedEventsDispatcher, given that this service doesn't dispatch workflow definitions itself, but rather, workflow definition related events, such as "a request to refresh activity registry", and potentially: "a request to evict the workflow definition cache".
A nice aspect to the proposed name is that it makes it explicitly clear that the purpose of this service is to distribute (publish) events to multiple subscribers.
| await workflowDefinitionDispatcher.DispatchAsync(new RefreshWorkflowDefinitionsRequest(), cancellationToken); | ||
| await activityRegistryPopulator.PopulateRegistryAsync(typeof(WorkflowDefinitionActivityProvider), cancellationToken); |
There was a problem hiding this comment.
The sender of this request will also receive it, which means it will re-populate the registry twice. Maybe awe might consider introducing MT middleware that includes a SenderID that stores the current application instance ID, so that it can then simply not deliver the message to the sender's consumer class. Populating the registry can potentially be a resource-intensive operation when a system has a large number of workflows, so it might be a worthwhile optimization. In any case, this would be out of scope for this PR, but would love to hear your thoughts.
There was a problem hiding this comment.
That might be an elegant and generic way to prevent duplicate execution. I wouldn't be surprised if MT is already adding the SenderID out of the box. We would need to make ignoring the message from the send optional though
…ssTransit project
| new(typeof(DateTimeOffset), PrimitivesCategory, | ||
| "A value type that consists of a DateTime and a time zone offset."), | ||
| new(typeof(TimeSpan), PrimitivesCategory, "Represents a duration of time."), | ||
| new(typeof(IDictionary<string, string>), LookupsCategory, "A dictionary with string key and values."), | ||
| new(typeof(IDictionary<string, object>), LookupsCategory, "A dictionary with string key and object values."), | ||
| new(typeof(ExpandoObject), DynamicCategory, "A dictionary that can be typed as dynamic to access members using dot notation.") | ||
| }; | ||
| new(typeof(ExpandoObject), DynamicCategory, | ||
| "A dictionary that can be typed as dynamic to access members using dot notation.") |
There was a problem hiding this comment.
Better to keep these on a single line.
Renamed `WorkflowDefinitionConsumer` to `WorkflowDefinitionEventsConsumer` to better reflect its purpose. Updated references accordingly in the `MassTransitWorkflowManagementFeature` class to align with the new name.
…' into feature/cross_node_cache_refresh
An unnecessary comment mark ("/") was removed in the definition of the DistributedWorkflowDefinitionNotificationsHandler class. This change contributes to code cleanup and increases code readability.
Three new destinations have been added to the LoadBalancer settings in Elsa Server. These are primarily intended to provide broader network coverage and improve overall server performance.
A new using directive for Elsa.MassTransit.Extensions has been included at the start of the script. In addition, under certain conditions, the system now uses the MassTransit Dispatcher in the workflow management system. These changes provide support for using MassTransit in the Elsa.Server component.
A new enum called MassTransitBroker has been added to represent different types of messaging brokers used in MassTransit. This has also been integrated into the Web project's Program.cs file to allow conditional usage of brokers in the MassTransit setup, enabling more flexible and dynamic configuration.
The WorkflowManagementFeature class has been cleaned up and its formatting has been corrected. A blank line was removed, a line break was added for readability in the AddVariableType method, and several stray spaces were also removed.
This commit removes unnecessary whitespace and improves the code readability in RefreshActivityRegistryHandler.cs. The changes include deletion of extra lines and adjustment of indentation.
This PR will ensure that all instances will keep an up-to-date cache ActivityRegistry