pubsub/aws/snssqs to support SQS dead-letter queue #1066

amimimor · 2021-08-09T18:47:41Z

Description

I've added 2 metadata fields which define:

the dead-letters queue (DLQ) name (mandatory)
and the max number of times a message is not acknowledged before it is moved to DLQ by SQS (optiona)

Once those fields are set the deletion of messages after the messageRetryLimit is not performed and the component assumes that the message is automatically moved by SQS to the defined DLQ.

Additionally, I've added an integration test that runs either with AWS directly or locally using localstack (sns,sqs,sts mockups) and tests 2 scenarios:

current default behavior (w/o DLQ)
new behavior with DLQ

Issue reference

#1065

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Created/updated tests
Extended the documentation / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]

This reverts commit bcaa9bb.

# This is the 1st commit message: Improve error message in case of missing property (dapr#1012) Co-authored-by: Artur Souza <[email protected]> # This is the commit message dapr#2: Remove vestigial pubsub/nats code (dapr#1024) The pubsub/nats component was replaced by pubsub/natsstreaming as part of dapr/dapr#2003, but the corresponding code in dapr/components-contrib was not removed, so this change removes it. # This is the commit message dapr#3: bugfix for sns topic deletion upon termination # This is the commit message dapr#4: Revert "bugfix for sns topic deletion upon termination" This reverts commit bcaa9bb.

…ts-contrib into sns-sqs-topics

artursouza · 2021-08-10T01:27:02Z

@halspang Please, check this PR with Matt.

* skip integration test if no AWS related envvars are set (skip in CI) * parallel testing in unittests

amimimor · 2021-08-10T13:59:02Z

hi @artursouza . My integration tests over AWS are failing with 'missing region' which indicates that other AWS related envvars are indeed present in the build context (somehow)(i've set t.Skip in cases where the credentials related envvars are missing). Am I right? If so, I need another AWS envvar, namely AWS_REGION to be set to a valid value in the test context

amimimor · 2021-08-10T14:01:23Z

hi @artursouza . My integration tests over AWS are failing with 'missing region' which indicates that other AWS related envvars are indeed present in the build context (somehow)(i've set t.Skip in cases where the credentials related envvars are missing). Am I right? If so, I need another AWS envvar, namely AWS_REGION to be set to a valid value in the test context

well, the tests seem to pass ok now (maybe I saw a cached result). thanks anyways and sorry for the bother

artursouza · 2021-08-12T01:01:17Z

pubsub/aws/snssqs/snssqs.go

 	messageRetryLimit int64
+	// if sqsDeadLettersQueueName is set to a value, then the deadLettersMaxReceives defines the number of times a message is received
+	// before it is moved to the dead-letters queue. This value must be smaller than messageRetryLimit
+	deadLettersMaxReceives int64


Can this be renamed to messageReceiveLimit?

sure. I borrowed it from the AWS official param. but your suggestion makes sense

artursouza · 2021-08-12T01:02:52Z

pubsub/aws/snssqs/snssqs.go

+	}
+
+	if val, ok := getAliasedProperty([]string{"deadLettersMaxReceives"}, metadata); !ok {
+		md.deadLettersMaxReceives = md.messageRetryLimit


Why not make Max Receive mandatory? Mixing these can be confusing.

artursouza · 2021-08-12T01:07:33Z

pubsub/aws/snssqs/snssqs_integ_test.go

@@ -0,0 +1,310 @@
+// ------------------------------------------------------------


Move these to the e2e folder until we have a standardized way to write integration tests. See this as example: https://github.com/dapr/components-contrib/tree/master/tests/e2e/bindings/zeebe

this is WIP and lots of work. So I think I'll open another PR for that. @artursouza wdyt? I've implemented the suggested changes (thanks, good ones!). I'm going to trigger a new build as it seem to glitch. please re-review

I am OK for the E2E test to be in a separate PR. Once we have integration tests, it can land there too.

* not using implicit maxReceives * maxReceives renamed * unittest refactor

artursouza

Just a small change. Plus, don't forget to remove the integ_test file.

artursouza · 2021-08-13T20:39:50Z

pubsub/aws/snssqs/snssqs.go


 const (
-	awsSqsQueueNameKey = "dapr-queue-name"
+	awsSqsQueueNameKey = "dapr-worker-name"


This change is a breaking change. This component is alpha but I would avoid breaking changes still.

sure. you're right

…-contrib into dead-letters

codecov · 2021-08-14T01:43:13Z

Codecov Report

Merging #1066 (e5d5b37) into master (253ef85) will decrease coverage by 0.50%.
The diff coverage is 23.40%.

@@            Coverage Diff             @@
##           master    #1066      +/-   ##
==========================================
- Coverage   34.53%   34.03%   -0.51%     
==========================================
  Files         132      134       +2     
  Lines       10870    11195     +325     
==========================================
+ Hits         3754     3810      +56     
- Misses       6736     6993     +257     
- Partials      380      392      +12

Impacted Files	Coverage Δ
authentication/azure/storage.go	`0.00% <0.00%> (ø)`
...indings/azure/servicebusqueues/servicebusqueues.go	`15.06% <0.00%> (-0.21%)`	⬇️
bindings/smtp/smtp.go	`53.33% <0.00%> (-1.84%)`	⬇️
pubsub/rocketmq/rocketmq.go	`0.00% <ø> (ø)`
pubsub/rocketmq/settings.go	`86.66% <ø> (ø)`
state/azure/tablestorage/tablestorage.go	`13.20% <0.00%> (ø)`
bindings/alicloud/tablestore/tablestore.go	`2.54% <2.54%> (ø)`
pubsub/aws/snssqs/snssqs.go	`26.57% <14.28%> (-2.42%)`	⬇️
bindings/aws/ses/ses.go	`24.05% <24.05%> (ø)`
state/redis/redis.go	`42.22% <33.33%> (-0.64%)`	⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 796df4d...e5d5b37. Read the comment docs.

* bugfix for sns topic deletion upon termination * Revert "bugfix for sns topic deletion upon termination" This reverts commit bcaa9bb. * wip on normalizing queue/topic names * sanitize queue and topic names * sanitized names. bugfix for close * # This is a combination of 4 commits. # This is the 1st commit message: Improve error message in case of missing property (dapr#1012) Co-authored-by: Artur Souza <[email protected]> # This is the commit message dapr#2: Remove vestigial pubsub/nats code (dapr#1024) The pubsub/nats component was replaced by pubsub/natsstreaming as part of dapr/dapr#2003, but the corresponding code in dapr/components-contrib was not removed, so this change removes it. # This is the commit message dapr#3: bugfix for sns topic deletion upon termination # This is the commit message dapr#4: Revert "bugfix for sns topic deletion upon termination" This reverts commit bcaa9bb. * removed debug message * raw string abort * merge issues solved * wip * gofmt+remove regex and use byte iter * wip. first impl of dead-letters queue config * wip. refactor and fallback values * integration test wip * wip integration test * wip integration * wip on testing * wip * still buggy but wip! * bugfix in dlq creation * working. still bug in subscription clean up * Update snssqs_integ_test.go * golangci-lint fixes * golangci-lint refactoring * trying to skip running integrations for snssqs * testing * skip integration test if no AWS related envvars are set (skip in CI) * parallel testing in unittests * code review fixes * not using implicit maxReceives * maxReceives renamed * unittest refactor * Update snssqs.go * integ removed, renaming back of const Co-authored-by: Maarten Mulders <[email protected]> Co-authored-by: Artur Souza <[email protected]> Co-authored-by: Dapr Bot <[email protected]> Signed-off-by: Amit Mor <[email protected]>

Amit Mor and others added 28 commits July 23, 2021 17:34

bugfix for sns topic deletion upon termination

bcaa9bb

Revert "bugfix for sns topic deletion upon termination"

62ac6b0

This reverts commit bcaa9bb.

wip on normalizing queue/topic names

de891cf

sanitize queue and topic names

d4d2df7

sanitized names. bugfix for close

d3e08e7

Merge branch 'sns-sqs-topics' of https://github.com/amimimor/componen…

f300ab3

…ts-contrib into sns-sqs-topics

removed debug message

408fdcd

raw string abort

f3bee8a

merge issues solved

be550bf

wip

7b7dc93

gofmt+remove regex and use byte iter

cb0f2e5

Merge branch 'sns-sqs-topics' into dead-letters

1c82e85

wip. first impl of dead-letters queue config

dd0c6e2

wip. refactor and fallback values

6c3e283

Merge remote-tracking branch 'upstream/master' into dead-letters

e38572a

integration test wip

b4692c9

wip integration test

58ad8a9

wip integration

46bbbc7

Merge remote-tracking branch 'upstream/master' into dead-letters

43b6e9e

Merge remote-tracking branch 'upstream/master' into dead-letters

582dfb8

wip on testing

3cfa704

Merge remote-tracking branch 'upstream/master' into dead-letters

c5301a6

wip

aace0a1

still buggy but wip!

f36e268

bugfix in dlq creation

eaf769e

working. still bug in subscription clean up

e88ae91

Update snssqs_integ_test.go

06c2ff2

amimimor requested review from a team as code owners August 9, 2021 18:47

golangci-lint fixes

207c203

Amit Mor added 3 commits August 10, 2021 14:58

golangci-lint refactoring

2b29a40

trying to skip running integrations for snssqs

98222d9

testing

a4b5c26

* skip integration test if no AWS related envvars are set (skip in CI) * parallel testing in unittests

artursouza suggested changes Aug 12, 2021

View reviewed changes

artursouza self-assigned this Aug 12, 2021

Amit Mor and others added 5 commits August 12, 2021 09:39

Merge remote-tracking branch 'upstream/master' into dead-letters

39ca9ba

code review fixes

d17746f

* not using implicit maxReceives * maxReceives renamed * unittest refactor

Update snssqs.go

fc73805

Merge remote-tracking branch 'upstream/master' into dead-letters

aca1b3e

Merge branch 'master' into dead-letters

ec5c12f

artursouza suggested changes Aug 13, 2021

View reviewed changes

Amit Mor added 2 commits August 13, 2021 23:46

integ removed, renaming back of const

eba168c

Merge branch 'dead-letters' of https://github.com/amimimor/components…

811c4b2

…-contrib into dead-letters

artursouza approved these changes Aug 14, 2021

View reviewed changes

Merge branch 'master' into dead-letters

2722455

artursouza added the automerge label Aug 14, 2021

dapr-bot added 2 commits August 13, 2021 17:47

Merge branch 'master' into dead-letters

8f4f6ab

Merge branch 'master' into dead-letters

e5d5b37

dapr-bot merged commit 284d744 into dapr:master Aug 14, 2021

msfussell mentioned this pull request Jan 4, 2022

SNS/SQS PubSub: missing sqsDeadLettersQueueName info dapr/docs#1899

Closed

		@@ -0,0 +1,310 @@
		// ------------------------------------------------------------

pubsub/aws/snssqs to support SQS dead-letter queue #1066

pubsub/aws/snssqs to support SQS dead-letter queue #1066

Uh oh!

Conversation

amimimor commented Aug 9, 2021

Description

Issue reference

Checklist

Uh oh!

artursouza commented Aug 10, 2021

Uh oh!

amimimor commented Aug 10, 2021

Uh oh!

amimimor commented Aug 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artursouza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 14, 2021

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants