-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Resolve Akka.Delivery and Akka.Cluster.Sharding.Delivery issues #7538
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
Resolve Akka.Delivery and Akka.Cluster.Sharding.Delivery issues #7538
Conversation
Fix akkadotnet#7529, Fix akkadotnet#7530: Fix ShardingProducerController and RetryTimer issues - Fix akkadotnet#7529: Update ShardingProducerController to return sequence numbers instead of Done.Instance in AskNextTo responses to match Task<long> return type - Fix akkadotnet#7530: Properly store and increment RetryTimer interval in ScheduleNext() method to enable exponential backoff This commit addresses two issues in the Akka.Cluster.Sharding.Delivery module: 1. Fixes type mismatch in ShardingProducerController where AskNextTo was returning Done.Instance instead of the expected sequence number 2. Fixes RetryTimer not properly incrementing intervals by storing the new interval value before starting the timer
Aaronontheweb
left a comment
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.
Detailed my changes
| case (_, _, { IsEmpty: false } replyTo): | ||
| replyTo.Value.Tell(Done.Instance); | ||
| case (_, outSeqNr, { IsEmpty: false } replyTo): | ||
| replyTo.Value.Tell(outSeqNr); // Send the sequence number instead of Done.Instance |
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.
Reply with the seqNo instead of Task.Done
| if (newInterval != Interval) | ||
| { | ||
| Interval = newInterval; // Store the new interval | ||
| Timers.StartPeriodicTimer(Retry.Instance, Retry.Instance, newInterval); |
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.
Use the correct backoff interval
|
|
||
| // expecting 3 end messages, one for each entity: "entity-0", "entity-1", "entity-2" | ||
| await consumerEndProbe.ReceiveNAsync(3, TimeSpan.FromSeconds(5)).ToListAsync(); | ||
| await consumerEndProbe.ReceiveNAsync(3, TimeSpan.FromSeconds(10)).ToListAsync(); |
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.
This test was racy so I took a look at the build logs and the issue was just that we didn't quite get through the 40ish messages we need to send for this assertion to pass, I assume because the Azure DevOps box is pretty tiny. Expanded the timeout here to give it a fighting chance.
Arkatufus
left a comment
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.
LGTM
Changes
Fix #7529, Fix #7530: Fix ShardingProducerController and RetryTimer issues
This commit addresses two issues in the Akka.Cluster.Sharding.Delivery module:
Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):