-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Workflow refresh feature #5713
Workflow refresh feature #5713
Conversation
Removed IDistributedWorkflowDefinitionEventsDispatcher interface and related classes in favor of direct message publishing through MassTransit. Introduced new workflow definition refresh endpoints and services for better modularity and responsiveness. Adjusted caching and notification systems to support the new architecture.
Updated the naming for clarity and consistency across the system. This change affects AmbientConsumerScope, related consumers, and condition checks to improve readability and understanding of the consumer context.
IConsumer<WorkflowDefinitionVersionsDeleted>, | ||
IConsumer<WorkflowDefinitionVersionsUpdated> | ||
public class WorkflowDefinitionEventsConsumer(IWorkflowDefinitionActivityRegistryUpdater workflowDefinitionActivityRegistryUpdater, INotificationSender notificationSender) : | ||
global::MassTransit.IConsumer<WorkflowDefinitionDeleted>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the global needed?
using Elsa.Workflows.Management.Contracts; | ||
using JetBrains.Annotations; | ||
using MassTransit; | ||
|
||
namespace Elsa.MassTransit.Consumers; | ||
|
||
/// <summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a new standard that we want to adhere to? Are we not to use summary tags for 1 liners, classes and or PublicAPI's?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking, the less noise on the screen, the better. Removing these optional tags help with that.
public Task HandleAsync(WorkflowDefinitionsRefreshed notification, CancellationToken cancellationToken) | ||
{ | ||
// Prevent re-entrance. | ||
if (AmbientConsumerScope.IsConsumerExecutionContext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the advantage of the distributed handler raising a local event and the local handler raising a distributed event, needing this scope to prevent loops? Also this does not look thread-safe to be reused for other events so we might want to make the naming more specific if we can not make it more reusable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, we want to distribute a given domain event throughout the cluster via MT. To that end, this handler will publish the notification as a message. The message consumers on the other pods, then, needs to raise the notification - except for the pod that published the message in the first place.
It seems to me that this is the easiest, most straightforward flow to reason about, but I am open to alternative approaches if they are simpler.
The current implementation isn't intended to be reused, so I agree that the ambient property might be renamed to something more specific.
@MariusVuscanNx perhaps in your next PR we can rename it to IsWorkflowDefinitionEventsConsumer
or similar.
var pageArgs = PageArgs.FromPage(currentPage, batchSize); | ||
var definitions = await store.FindManyAsync(filter, order, pageArgs, cancellationToken); | ||
|
||
if (definitions.Items.Count == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also add a check for count < batchSize. That will prevent an extra call to the database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, nice. @MariusVuscanNx can we incorporate that in your next PR?
|
||
var secondResponse = await client.GetStringAsync("second-value"); | ||
|
||
Assert.Equal("", firstResponse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not check against the response code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What code would you check against? The endpoint always responds with 200 OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is checking if the new endpoint is set up within the triggers. If this is not the case I believe a 404 should be returned, not a 200.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes you are right. @MariusVuscanNx what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method perform an http GET request and tries to retrieve the content (string). If case it doesn't work a 404 will be returned and an exception will be thrown.
In short it will work only if we have a successful status code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. But then, I wonder, if it wouldn't be more reflective of the test to do e.g. client.SendAsync()
, receive the HttpResponseMessage
and then explicitly check for its StatusCode
to be 200.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the code to check the status code.
This change is