-
Notifications
You must be signed in to change notification settings - Fork 586
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
Add the ability to cancel scheduled messages #746
Add the ability to cancel scheduled messages #746
Conversation
Looking good, @ErikMogensen. A few questions/suggestions. |
Co-authored-by: Sean Feldman <SeanFeldman@users.noreply.github.com>
…nsen/ServiceBusExplorer into CancelScheduledMessages
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'm not going to block, but I'd like you to double-check a couple of comments/suggestions. Thanks, Erik!
{ | ||
if (null != sequenceNumber) | ||
{ | ||
await queueClient.CancelScheduledMessageAsync((long)sequenceNumber); |
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.
When we get the sequence numbers into a list, that's when we can cast to long
.
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.
See comment here.
… HandleQueueControl.cs
@SeanFeldman, there were quite a few changes since your review. Do you want to take another look? When I limited it to 40 parallel tasks it is quite fast and works 😄 I also added a new context menu item for selecting all rows Selected a single, normal messages: Selected multiple, normal messages: |
Co-authored-by: Sean Feldman <SeanFeldman@users.noreply.github.com>
Co-authored-by: Sean Feldman <SeanFeldman@users.noreply.github.com>
Some thoughts about this PR in light of some benchmarking that was done. It is worth mentioning that this is related to #360. We closed the issue as I also benchmarked an alternative approach, which is more intrusive, using auto-forwarding and a temporary queue. Assuming there's a While I still rather see ASB supporting purging (scenario when there are no inflight messages or those don't matter) and ability to peek messages by status, for large quantities of scheduled messages, the auto-forwarding option might be better suited. Food for thought |
Fixed it to build and handle failed cancellations. Tested with 100.000 messages and it took a bit less than ten minutes to cancel all of them. @SeanFeldman, interesting solution. What do you mean by:
|
I meant PeekLock and complete. |
Do you mean the solution I created and you just reviewed? It is using Peek to find out status and then CancelScheduledMessages on the scheduled messages. |
Yes. There's no other option today. Except the one I mentioned but it's not "safe". |
@SeanFeldman, do you approve of this PR now? |
👍 |
Fixes #360
This PR adds the ability to cancel scheduled messages on queues. The Service bus API does not support browsing scheduled messages on subscriptions.
A new context menu item "Cancel Scheduled Message" has been added as shown below:
If multiple messages are selected it says "Cancel Scheduled Messages" instead:
If the Disable Accidental Deletion Prevention is disabled, a message box will ask for confirmation.
The rows in the grid will not update, nor will the message count for the queue in the tree view. That is something that may be added later.