Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(sns): fifo topic with content-based deduplication support #11127 #11588

Merged
merged 11 commits into from
Jan 4, 2021
Merged

feat(sns): fifo topic with content-based deduplication support #11127 #11588

merged 11 commits into from
Jan 4, 2021

Conversation

rrhodes
Copy link
Contributor

@rrhodes rrhodes commented Nov 19, 2020

Add support for FIFO and content-based deduplication in SNS topics.

Closes #11127

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Nov 19, 2020 •
edited
Loading

@rrhodes
Copy link
Contributor Author

rrhodes commented Nov 25, 2020

Hi @MrArnoldPalmer, any feedback for this PR?

tombuckley91
tombuckley91 reviewed Nov 29, 2020
View reviewed changes
Copy link

@tombuckley91 tombuckley91 left a comment

Choose a reason for hiding this comment

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

I understand you have called the prop "fifoTopic" to correspond with the underlying cfn, however, the sqs module has this feature implemented with the prop "fifo". In the interest of consistency, would you consider changing the prop name to "fifo"?

Thanks for your work on this, definitely waiting on this feature!

@rrhodes
Copy link
Contributor Author

rrhodes commented Dec 3, 2020

Hi @MrArnoldPalmer, @tombuckley91, any further feedback regarding this PR?

MrArnoldPalmer
MrArnoldPalmer previously requested changes Dec 7, 2020
View reviewed changes
Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer left a comment

Choose a reason for hiding this comment

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

Additionally we should add another test case to cover fifo: true + contenBasedDeduplication: true

packages/@aws-cdk/aws-sns/lib/topic.ts
@@ -70,6 +84,8 @@ export class Topic extends TopicBase {
displayName: props.displayName,
topicName: this.physicalName,
kmsMasterKeyId: props.masterKey && props.masterKey.keyArn,
contentBasedDeduplication: props.contentBasedDeduplication,
Copy link
Contributor

Choose a reason for hiding this comment

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

So reading the docs it seems contentBaseddeduplication can only be true when fifo is true. That being the case in L2s we prefer to check for this during synth. IE: if a user passed contentBasedDeduplication: true without setting fifo: true, we should throw an error with a message like "contentDeduplication can only be enabled for fifo topics."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this feedback @MrArnoldPalmer, I'll amend this later today.

packages/@aws-cdk/aws-sns/test/test.sns.ts Outdated
@@ -103,6 +103,48 @@ export = {

test.done();
},

'specify contentBasedDeduplication'(test: Test) {
Copy link
Contributor

Choose a reason for hiding this comment

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

With regards to comment on contentBasedDeduplication, this test should actually assert that an error is thrown since fifo is not enabled.

@mergify mergify bot dismissed MrArnoldPalmer’s stale review December 7, 2020 18:22

Pull request has been modified.

@rrhodes
Copy link
Contributor Author

rrhodes commented Dec 9, 2020

Hi @MrArnoldPalmer, any further feedback following my latest changes?

@rrhodes
Copy link
Contributor Author

rrhodes commented Dec 17, 2020

Hi @MrArnoldPalmer @tombuckley91, any further feedback for this PR?

@rrhodes
Copy link
Contributor Author

rrhodes commented Jan 2, 2021

Hi @MrArnoldPalmer, any further feedback for this PR? Anyone else I can request to review this?

MrArnoldPalmer
MrArnoldPalmer approved these changes Jan 4, 2021
View reviewed changes
Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

TY @rrhodes! Thanks for your patience.

@mergify
Copy link
Contributor

mergify bot commented Jan 4, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify
Copy link
Contributor

mergify bot commented Jan 4, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 7e60d8e into aws:master Jan 4, 2021
@rrhodes rrhodes deleted the feature/11127-sns-fifo-topic branch January 4, 2021 16:27
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 0fe2438
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

flochaz pushed a commit to flochaz/aws-cdk that referenced this pull request Jan 5, 2021
@rrhodes @flochaz
feat(sns): fifo topic with content-based deduplication support aws#11127
a5189d0 
 (aws#11588)

Add support for FIFO and content-based deduplication in SNS topics.

Closes aws#11127 
----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tombuckley91 tombuckley91 tombuckley91 left review comments

@MrArnoldPalmer MrArnoldPalmer MrArnoldPalmer approved these changes

Assignees

@MrArnoldPalmer MrArnoldPalmer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[aws-sns] SNS FIFO
4 participants
@rrhodes @aws-cdk-automation @MrArnoldPalmer @tombuckley91