AWS SQS Stream Provider #2053
AWS SQS Stream Provider #2053
Conversation
| @@ -0,0 +1,421 @@ | |||
| | |||
There was a problem hiding this comment.
Seems like return of the query from #2038
|
@dVakulen sorry, now it is properly rebased. |
|
@jason-bragg since you was playing with Streams recently, do you mind review this as well? Thanks |
| /// <param name="queueName">The name of the queue</param> | ||
| /// <param name="connectionString">The connection string</param> | ||
| /// <param name="deploymentId">The cluster deployment Id</param> | ||
| public SQSStorage(string queueName, string connectionString, string deploymentId = "") |
There was a problem hiding this comment.
Separation of concerns -
Parsing fields out of some configuration data should not be part of this class. Construct with actual parameters or with interface for configuration class. Parsing should be done between configuration and this class, not part of this class.
There was a problem hiding this comment.
Although it is the kind of change that IMHO is pure cosmetic and just a judgement call since there is no real benefit on that (as you can see on SQL connections which are pure strings), I'll make this change not just here but on the DynamoDBStorage as well, since their connection strings are basically the same. I'll do that in another PR before we release the nuget package itself for AWSUtils among other cosmetic issues.
|
@galvesribeiro Thanks for working on this. Streaming support for AWS is a great addition. This stream provider looks based off of the AzureQueue stream provider. Azure queues are unordered and can introduce duplicates, does SQS have the same characteristics? I ask because ordered queues like Kafka or EventHub should be handled different, and I want to be somewhat confident we're setting up this provider in a way that is appropriate for it's data. |
|
Yes, SQS don't guarantee order. SQS has mostly the same feature as well as the limitation on Azure Queues and that is why the code is almost copy/pasted from Azure. I plan to have a refactory later to make a "GenericQueueStreamProvider" where the only thing that would basically change, is the backing Queue. Something like EventHub and Kafka on AWS, is called Kinesis, and is other provider type that someone else in the community is already working and will should put a PR soon. Bear with me that the cosmetic changes that you suggested will be address in a final PR that will add more descriptive error codes and some other cosmetic change not just to this PR but to all the other providers as I already agreed with the other reviewers of the AWSUtils. Thanks for take time to review @jason-bragg, I'll push the commit with the changes in a few minutes. |
|
@jason-bragg the code config on the tests is the only feedback I intentionally didn't made the changes from your feedback because before we release the beta nuget of AWSUtils, I'll move all the tests to a separated project as @jdom suggested on other thread, so there I'll use code config. Is there anything else? This is the last PR for this bare minimal set of providers since all the other were merged. Thanks |
|
Thanks @jason-bragg Will put another PR with the remaining fixes soon. |
|
Thank you, @galvesribeiro, for pulling this off! |
Implementation of #2006 which is part of #2005