Skip to content

Conversation

danielmarbach
Copy link
Contributor

@danielmarbach danielmarbach commented Sep 25, 2024

Follow through on #1049

This attempts to simplify the dispatcher a bit where possible to no longer rely on the synchronous path. This PR attempts to find a good balance between allocating the necessary task lists with the number of items needed because that is beneficial compared to doing Task.WhenAll with Select and ToArray

image

During the discussion with @andreasohlund and @PhilBastian we concluded the original problem would only really have been discovered if the following are true:

  • Premium namespace (we don't use a premium namespace yet in our CI/CD environment)
  • The underlying SDK cannot yet have cached the batch size information of the underlying AMQP link (will make the CreateMessageBatchAsync yield) or the large message has to be put into a batch that is sent after the first batch was dispatched

Given that we have not even the premium namespace in place currently, simplifying the dispatcher slightly to avoid such an error seems to be warranted.

I have also removed the yielding, since that was just a way to reproduce the previous problem. With the new structure this is no longer necessary.

if (!defaultOperationsPerDestination.ContainsKey(destination))
{
defaultOperationsPerDestination[destination] = [operation];
// because we batch only the number of destinations are relevant
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting rid of this here also reduces the cognitive overhead because this code doesn't have knowledge anymore about the inner workings of the batching path.

@danielmarbach danielmarbach merged commit 9ddaa06 into master Sep 26, 2024
3 checks passed
@danielmarbach danielmarbach deleted the simplify-dispatcher branch September 26, 2024 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants