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.

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

Feat: Add SNS FIFO Topic support #130

Conversation

codingtim
Copy link
Contributor

@codingtim codingtim commented May 5, 2021

Fixes #41

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

This pull requests add support for FIFO SNS publishRequest. #41
The implementation is based on QueueMessageChannel.

💡 Motivation and Context

SNS added FIFO topics which require a mandatory MessageGroupId when publishing messages.
Without this change it is not possible to use spring-cloud-aws to publish messages to SNS FIFO topics.

💚 How did you test it?

Unit tests were added.

📝 Checklist

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

🔮 Next steps

@maciejwalkowiak maciejwalkowiak self-assigned this May 31, 2021
@maciejwalkowiak maciejwalkowiak added the component: sns SNS integration related issue label May 31, 2021
@maciejwalkowiak maciejwalkowiak modified the milestones: 2.3.2, 2.3.x May 31, 2021
@maciejwalkowiak maciejwalkowiak added the type: enhancement Smaller enhancement in existing integration label May 31, 2021
/**
* Message group id for SNS message (applies only to FIFO queue).
*/
public static final String MESSAGE_GROUP_ID_HEADER = "MessageGroupId";
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this value be the same as the SQS headers in SqsMessageHeaders.java?

Suggested change
public static final String MESSAGE_GROUP_ID_HEADER = "MessageGroupId";
public static final String MESSAGE_GROUP_ID_HEADER = "message-group-id";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct that this does not follow the naming of SqsMessageHeaders. I followed the AWS header naming. I do not see added value in adding another translation layer which is confusing to users?
I also understand the need for consistency so maintainer can say which is preferred :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I would be in for keeping consistency with AWS naming, but since SQS has been implemented already like that 4 years ago, lets keep it in sync with SQS implementation.

We can reconsider for 3.0

/**
* Message Deduplication id for SNS message.
*/
public static final String MESSAGE_DEDUPLICATION_ID_HEADER = "MessageDeduplicationId";
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as before

Suggested change
public static final String MESSAGE_DEDUPLICATION_ID_HEADER = "MessageDeduplicationId";
public static final String MESSAGE_DEDUPLICATION_ID_HEADER = "message-deduplication-id";

@codingtim
Copy link
Contributor Author

@maciejwalkowiak can you have a look at the first remark of Alex and say which value should be used for the headers? Then I update the PR so it can be merged for an upcoming release?

…saging/core/TopicMessageChannel.java

Co-authored-by: Alex Moreno <83343424+alex-ninetynine@users.noreply.github.com>
@maciejwalkowiak
Copy link
Contributor

@codingtim thanks for the PR! Are you also able to update the reference docs? It would be great to add a section explaining how to use FIFO SNS topics.

@maciejwalkowiak maciejwalkowiak added the status: waiting-for-feedback Waiting for feedback from issuer label Jun 17, 2021
@github-actions github-actions bot added the type: documentation Documentation or Samples related issue label Jun 17, 2021
@codingtim
Copy link
Contributor Author

@maciejwalkowiak reference docs are updated to include FIFO SNS topic

@birnbuazn
Copy link

Any news on this? Unfortunately Spring Cloud AWS seems to be moving very slowly. This is becoming more and more of a show stopper for us, because we are already waiting for 2 PRs to be approved and merged.

@maciejwalkowiak
Copy link
Contributor

@birnbuazn we're working now towards release. It does move slower than we wish, we are working on finding more time to work on the project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: sns SNS integration related issue status: waiting-for-feedback Waiting for feedback from issuer type: documentation Documentation or Samples related issue type: enhancement Smaller enhancement in existing integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for AWS SNS FIFO topics
4 participants