bug: Support async sns/sqs message publishing v9 #3269
Conversation
| @@ -45,16 +45,16 @@ public AWSMessagingGateway(AWSMessagingGatewayConnection awsConnection) | |||
| _awsClientFactory = new AWSClientFactory(awsConnection); | |||
| } | |||
|
|
|||
| protected string EnsureTopic(RoutingKey topic, SnsAttributes attributes, TopicFindBy topicFindBy, OnMissingChannel makeTopic) | |||
| protected async Task<string> EnsureTopic(RoutingKey topic, SnsAttributes attributes, TopicFindBy topicFindBy, OnMissingChannel makeTopic) | |||
There was a problem hiding this comment.
Since this is a v9 PR, you need to be careful about breaking changes. Changing the signature of a non-private method on a non-private class is a breaking change for consumers updating their packages - they'd now have a bunch of tasks flying around in their code they're not going anything with.
Breaking changes should be reserved for major version upgrades, so v10 would be an OK place to make changes like this. That said, it would need to be part of a broader conversation around the conventions when it comes to supporting async methods. I would suggest we keep it as the addition of the async publish methods, and leave the auxiliary methods as they are now.
| @@ -42,7 +43,7 @@ public SqsMessagePublisher(string topicArn, AmazonSimpleNotificationServiceClien | |||
| _client = client; | |||
| } | |||
|
|
|||
| public string Publish(Message message) | |||
| public async Task<string> Publish(Message message) | |||
There was a problem hiding this comment.
As with the comment above about breaking changes, since this is a public method on a public class, we should be adding a new method for the async version, even if the sync version is just calling GetAwaiter().GetResult() on the new method.
This reverts commit 13f02f1.
jtsalva
changed the title
| @@ -61,7 +61,7 @@ public SqsMessageProducerSendTests() | |||
| public async Task When_posting_a_message_via_the_producer() | |||
| { | |||
| //arrange | |||
| _messageProducer.Send(_message); | |||
There was a problem hiding this comment.
We should probably have one version of the test that calls the sync method, and one version that calls the async method.
jtsalva
requested review from
iancooper,
holytshirt,
DevJonny and
preardon
as code owners
jtsalva
changed the title
Addresses #3242