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

[DEVPLAT-1850] Add SQS support to PubSub #127

Merged
merged 6 commits into from
Nov 8, 2024

Conversation

Neurostep
Copy link
Contributor

@Neurostep Neurostep commented Nov 1, 2024

Description

The MLD team is building the asynchronous architecture based on the AWS SQS and wants to integrate it into the Doc Chat service. We have to add the SQS PubSub interface to the go-sdk to support that.

In this PR we add support for SQS transport in our PubSub system:

  • Add SQS PubSub client wrappers
  • Add go-kit SQS transport
  • Update PubSub configuration documentation

NOTE

I stumbled into the problem with the AWS GO SDK v2 issue described here: aws/aws-sdk-go-v2#2370

The solution is to upgrade the aws-sdk-go-v2 and all dependent packages to the latest version. I didn't notice any other breaking changes that we should take care of. Eventually, we will upgrade Chassis services using Parity workflow

Testing considerations

Checklist

  • Prefixed the PR title with the JIRA ticket code
  • Performed simple, atomic commits with good commit messages
  • Verified that the commit history is linear and commits are squashed as necessary
  • Thoroughly tested the changes in development and/or staging
  • Updated the README.md as necessary

Related links

@Neurostep Neurostep force-pushed the maksimt/DEVPLAT-1850/pubsub-sqs branch 11 times, most recently from 504f95a to 9a1909d Compare November 5, 2024 16:38
@Neurostep Neurostep marked this pull request as ready for review November 5, 2024 17:13
@Neurostep Neurostep requested a review from a team as a code owner November 5, 2024 17:13
@Neurostep Neurostep requested review from terranisu and laynax November 5, 2024 17:13
pkg/pubsub/sqs/subscriber.go Outdated Show resolved Hide resolved
pkg/pubsub/sqs/subscriber.go Show resolved Hide resolved
pkg/pubsub/sqs/subscriber.go Outdated Show resolved Hide resolved
pkg/pubsub/sqs/subscriber.go Show resolved Hide resolved
README.md Show resolved Hide resolved
@Neurostep Neurostep force-pushed the maksimt/DEVPLAT-1850/pubsub-sqs branch from 9a1909d to d2ae7ce Compare November 7, 2024 11:08
@Neurostep Neurostep requested a review from laynax November 7, 2024 11:10
laynax
laynax previously approved these changes Nov 7, 2024
Copy link
Contributor

@laynax laynax left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 📜

I still think in case of an interrupt we will lose some messages mid way, but we can improve it in the future

@Neurostep Neurostep force-pushed the maksimt/DEVPLAT-1850/pubsub-sqs branch from d2ae7ce to aac5282 Compare November 7, 2024 16:52
@Neurostep Neurostep requested a review from laynax November 7, 2024 16:52
@Neurostep Neurostep force-pushed the maksimt/DEVPLAT-1850/pubsub-sqs branch from 048282f to 617cb65 Compare November 7, 2024 17:25
@Neurostep
Copy link
Contributor Author

@laynax

still think in case of an interrupt we will lose some messages mid way, but we can improve it in the future

Do you have something in particular in mind? I tested it with the service restart, and all looks 👌

In AWS SQS, if the message is not deleted from the queue, it will be re-queued again after the timeout (default 30 seconds).

laynax
laynax previously approved these changes Nov 8, 2024
@laynax
Copy link
Contributor

laynax commented Nov 8, 2024

@laynax

still think in case of an interrupt we will lose some messages mid way, but we can improve it in the future

Do you have something in particular in mind? I tested it with the service restart, and all looks 👌

In AWS SQS, if the message is not deleted from the queue, it will be re-queued again after the timeout (default 30 seconds).

Yeah but we better not to rely on the retention config. What I had in mind was something simple like having a waitgroup and returning from Unsubscribe() when the waitgroup is done.
Or a more complex, wait for the channel with tasks in the pool to be emptied and then return from Unsubscribe()
Again, we can leave it for later :) just wanted to have it here for posterity

@Neurostep Neurostep force-pushed the maksimt/DEVPLAT-1850/pubsub-sqs branch from 617cb65 to ba9ce58 Compare November 8, 2024 15:09
@Neurostep Neurostep requested a review from laynax November 8, 2024 15:11
@Neurostep
Copy link
Contributor Author

@laynax

Yeah but we better not to rely on the retention config. What I had in mind was something simple like having a waitgroup and returning from Unsubscribe() when the waitgroup is done.

This is make sense and I think we can easily achieve this 👍 Please, check out the new version 🙏

laynax
laynax previously approved these changes Nov 8, 2024
Copy link
Contributor

@laynax laynax left a comment

Choose a reason for hiding this comment

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

LGTM

@Neurostep Neurostep merged commit a016bc3 into main Nov 8, 2024
6 checks passed
@Neurostep Neurostep deleted the maksimt/DEVPLAT-1850/pubsub-sqs branch November 8, 2024 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants