-
Notifications
You must be signed in to change notification settings - Fork 302
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
Adding RaiseEvent to client for scalability, Allow creating queue with specific size #70
Conversation
I also added ability to set the maximum service bus queue size (issue #68) |
Also fixed issue #73 which was throwing a null ref exception when a work item was aborted. |
} | ||
|
||
private static readonly long[] ValidQueueSizes = { 1024L, 2048L, 3072L, 4096L, 5120L }; |
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.
convention is to not include modifier for private
var executionStartedEvent = message.Event as ExecutionStartedEvent; | ||
if (executionStartedEvent != null) | ||
var brokeredMessages = new BrokeredMessage[messages.Length]; | ||
for (int i = 0; i < messages.Length; i++) |
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.
GetBrokeredMessageFromObjectAsync could end up going to blob storage or another slow external storage so running this as tasks would be better so the latency is not sequential.
@@ -61,6 +61,11 @@ public ServiceBusOrchestrationServiceSettings() | |||
public int MaxTrackingDeliveryCount { get; set; } | |||
|
|||
/// <summary> | |||
/// Maximum queue size, in megabytes, for the service bus queues | |||
/// </summary> | |||
public long MaxQueueSizeInMegabytes { get; set; } = 1024; |
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.
1024L as default for consistency with the valid sizes list
/// <param name="instanceId">Instance id for the orchestration to be created, must be unique across the Task Hub</param> | ||
/// <param name="eventName">Name of the event</param> | ||
/// <param name="eventData">Data for the event</param> | ||
public async Task RaiseEventAsync( |
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 think the naming of RaiseEventAsync here is a bit confusing as the meaning for an already running instance is quite different.
I think it would be better to have a CreateOrchestrationInstanceWithRaisedEventAsync (open to other naming suggestions) and have the various permutations for with/without tags etc.
Likely best to have an InternalCreateOrchestrationInstanceAsync that this and the CreateOrchestrationInstanceAsync can call to avoid duplication.
A common pattern for long-running orchestrations is to create an orchestration then raise an event on it. This causes twice as many messages to service bus. This change will do this process in a batch send.