Skip to content

Conversation

@jwilmoth-nc
Copy link
Contributor

📢 Type of change

  • [X ] Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Conditionally add a random UUID dedup ID if the queue isn't configured for content based deduplication to allow consumers who are using AWS SQS's content based deduplication functionality to let AWS decide what's a duplicate.

💡 Motivation and Context

Fixes an issue with the same content being delivered via a FIFO queue configured with content based deduplication.

💚 How did you test it?

Added unit test.

📝 Checklist

  • I reviewed submitted code
  • [X ] I added tests to verify changes
  • I updated reference documentation to reflect the change
  • [ X] All tests passing
  • No breaking changes

🔮 Next steps

@tomazfernandes
Copy link
Contributor

Thanks for the PR @jwilmoth-nc!

There seems to be a lot of formatting changes, which makes the review harder - can you please run mvn verify so the spotless plugin can apply out default format?

@github-actions github-actions bot added the component: s3 S3 integration related issue label May 5, 2023
Copy link
Contributor

@tomazfernandes tomazfernandes left a comment

Choose a reason for hiding this comment

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

@jwilmoth-nc the PR looks great, thanks!

I've left a couple of comments - please let me know if they make sense to you.

@tomazfernandes
Copy link
Contributor

@maciejwalkowiak - I've asked you before but I don't quite remember what's the answer.

Whenever we run maven we get updates for S3 classes such as AbstractCrossRegionS3Client.java.

What should we do about those?

Seems a bit odd to update such classes in a PR related to SQS.

@maciejwalkowiak
Copy link
Contributor

@tomazfernandes run make format afterwards. Maybe it can be automated but this is currently the only solution

@tomazfernandes
Copy link
Contributor

@jwilmoth-nc we’ll shortly review and merge your PR, thanks.

I have one question just in case - have you confirmed targeting AWS that adding a messageDeduplicationId overrides content-based deduplication on AWS and that it does mess up deduplication?

AWS has a tendency to ignore unexpected things, so I wouldn’t be surprised if that was the case on this one.

I’m asking this because I couldn’t find anything that specific on the docs, just to measure impact.

Thanks again.

@jwilmoth-nc
Copy link
Contributor Author

@jwilmoth-nc we’ll shortly review and merge your PR, thanks.

I have one question just in case - have you confirmed targeting AWS that adding a messageDeduplicationId overrides content-based deduplication on AWS and that it does mess up deduplication?

AWS has a tendency to ignore unexpected things, so I wouldn’t be surprised if that was the case on this one.

I’m asking this because I couldn’t find anything that specific on the docs, just to measure impact.

Thanks again.

Hey @tomazfernandes thanks for the quick review cycle. Yes, here's a couple of screenshots of a fifo queue with content based deduplication set and two identical messages sent w/ different MessageDeduplicationId. Here's the AWS docs that say "If the queue has ContentBasedDeduplication set, your MessageDeduplicationId overrides the generated one"

image

image

The above messages were sent with:

aws --profile direct sqs send-message --queue-url https://sqs.us-east-1.amazonaws.com/****/spring-cloud-aws-issue-796-no-dedup.fifo --message-body "gh-issue-796" --message-group-id "test" --message-deduplication-id "1"
and
aws --profile direct sqs send-message --queue-url https://sqs.us-east-1.amazonaws.com/****/spring-cloud-aws-issue-796-no-dedup.fifo --message-body "gh-issue-796" --message-group-id "test" --message-deduplication-id "2"

@tomazfernandes
Copy link
Contributor

Cool, thanks for the info @jwilmoth-nc.

@maciejwalkowiak feel free to review / merge / polish anytime.

I should be able to take a closer look into this later today if that’s better.

Thanks!

Copy link
Contributor

@tomazfernandes tomazfernandes left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @jwilmoth-nc!

@maciejwalkowiak maciejwalkowiak merged commit c1b850a into awspring:main May 10, 2023
@maciejwalkowiak maciejwalkowiak added this to the 3.0.1 milestone May 10, 2023
@maciejwalkowiak maciejwalkowiak added type: bug Something isn't working and removed component: s3 S3 integration related issue labels May 10, 2023
@igormq igormq mentioned this pull request Nov 24, 2025
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: sqs SQS integration related issue type: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants