feat: bulk dispatch to support circuit breaking#3688
Conversation
|
Michael Freeman seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
There was a problem hiding this comment.
No application code in the PR — skipped Code Health checks.
See analysis details in CodeScene
Quality Gate Profile: Clean Code Collective
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
|
@iancooper Added a proposal for circuit breaking to support bulk dispatching. I'll continue on the premise that this implementation is ok and make changes if we have differing opinions. |
iancooper
left a comment
iancooper
left a comment
There was a problem hiding this comment.
Looks good, let's give it a shot. I've added to V10 as its a breaking change
There was a problem hiding this comment.
Gates Failed
(1 hotspot with Complex Method)
(1 file with Complex Method)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| OutboxProducerMediator.cs | 1 rule in this hotspot | 7.16 → 6.97 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| OutboxProducerMediator.cs | 1 advisory rule | 7.16 → 6.97 | Suppress |
Quality Gate Profile: Clean Code Collective
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
| private async Task BulkDispatchAsync( | ||
| IEnumerable<Message> posts, | ||
| RequestContext requestContext, | ||
| bool continueOnCapturedContext, |
There was a problem hiding this comment.
❌ New issue: Complex Method
BulkDispatchAsync has a cyclomatic complexity of 10, threshold = 9
mikechrisfreeman
changed the title
There was a problem hiding this comment.
Gates Failed
(1 hotspot with Complex Method)
(1 file with Complex Method)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| OutboxProducerMediator.cs | 1 rule in this hotspot | 7.16 → 6.97 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| OutboxProducerMediator.cs | 1 advisory rule | 7.16 → 6.97 | Suppress |
Quality Gate Profile: Clean Code Collective
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
| /// Class MessageBatch | ||
| /// A collection of T <see cref="Message"/> | ||
| /// </summary> | ||
| public interface IAmAMessageBatch<out T> : IAmAMessageBatch |
There was a problem hiding this comment.
This abstraction allows for vendor specific batching to be implemented.. Currently our batching implementation is quite primitive - it fetches a batch of messages from the outbox and then attempt to batch them up, not considering whether it exceeds the vendor specific batching limitations.
We intend to add a further PR to fix this, for now its setting it up for that use case.
| /// <exception cref="NotImplementedException"></exception> | ||
| public async Task SendAsync(IAmAMessageBatch batch, CancellationToken cancellationToken) | ||
| { | ||
| if (batch is not MessageBatch messageBatch) |
There was a problem hiding this comment.
An azure specific message batch will be implemented later so that we can fine tune vendor specific constraints
mikechrisfreeman
requested review from
DevJonny,
holytshirt and
preardon
as code owners
mikechrisfreeman
changed the title
There was a problem hiding this comment.
Gates Failed
(1 hotspot with Complex Method)
(1 file with Complex Method)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| OutboxProducerMediator.cs | 1 rule in this hotspot | 7.16 → 6.97 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| OutboxProducerMediator.cs | 1 advisory rule | 7.16 → 6.97 | Suppress |
Quality Gate Profile: Clean Code Collective
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
preardon
left a comment
preardon
left a comment
There was a problem hiding this comment.
Looks good to me, thanks for getting this in
|
@mikechrisfreeman Let us know when this is done, and I'll review |
There was a problem hiding this comment.
Gates Failed
(1 hotspot with Complex Method)
(1 file with Complex Method)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| OutboxProducerMediator.cs | 1 rule in this hotspot | 7.16 → 6.97 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| OutboxProducerMediator.cs | 1 advisory rule | 7.16 → 6.97 | Suppress |
Quality Gate Profile: Clean Code Collective
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
Thanks @iancooper - Updated from main, resolved merge conflicts and ready for review. I did notice that RocketMessageProducerFactory hasn't been updated to use the new IAmAMessageProducerFactory interface. Though i couldn't see any tests failing as a result. :/ Is that intentional? |
iancooper
changed the title
* docs: update adr to include circuit breaking proposal for bulk dispatching * feat: bulk dispatch to support circuit breaking * fix: merge from main --------- Co-authored-by: Michael Freeman <michael.freeman@flagstoneim.com> Co-authored-by: Paul Reardon <Paul@ReardonTech.UK>
5 participants
Currently only
AzureServiceBusMessageProducerandInMemoryProduceimplementIAmABulkMessageProducerAsync.The current behaviour batches messages per topic and batch size then publishes the batch. If one batch fails an exception is raised and successful batches are not recorded. This therefore renders the
MarkDispatchedAsyncon successful messages useless.Proposal is to bring the Bulk dispatch inline with the single dispatch implementation so that it can be wrapped in a retry and make use of
IAmAnOutboxCircuitBreaker. A further benefit is individual batches can be retried as opposed to the entire list of messages and better control over implementation specific batching, asb can be quite funny.