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

[ETL-621] Add SNS topic to broadcast new exports #106

Merged
merged 3 commits into from
Feb 26, 2024
Merged

[ETL-621] Add SNS topic to broadcast new exports #106

merged 3 commits into from
Feb 26, 2024

Conversation

philerooski
Copy link
Contributor

Lot's of files touched here, but essentially I've inserted an SNS topic between our S3 input bucket and our SQS queues (now there's two queues!) in anticipation of triggering both input -> intermediate and input -> raw processes in reaction to new export data. That looks sort of like this:

image
  • templates/sqs-queue.yaml was modified to subscribe to the SNS topic
  • src/lambda_function/s3_to_glue/app.py was modified to work with events formatted as SQS(SNS(S3))
  • config/prod/namespaced/s3-event-config-lambda.yaml was modified to add the S3 event notification for the SNS topic, rather than the SQS queue.

Renamed files:

  • config/develop/namespaced/s3-event-config-lambda.yaml renamed to config/develop/namespaced/lambda-s3-event-config.yaml to match the pattern {service_name}-{descriptor}
  • config/develop/namespaced/s3-event-config-lambda-role.yaml renamed to config/develop/namespaced/lambda-s3-event-config-role.yaml
  • etc. for prod

New files:

  • config/develop/namespaced/sns-topic.yaml
  • config/prod/namespaced/sns-topic.yaml
  • templates/sns-topic.yaml

Copy link

sonarcloud bot commented Feb 21, 2024

Quality Gate Passed Quality Gate passed

Issues
23 New issues

Measures
1 Security Hotspot
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

@BryanFauble BryanFauble left a comment

Choose a reason for hiding this comment

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

🔥 Changes look good to me!

Copy link
Contributor

@rxu17 rxu17 left a comment

Choose a reason for hiding this comment

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

LGTM! Were some of the main reasons for us moving from S3 -> SQS to S3 -> SNS -> SQS due to being able to decouple our S3 bucket from SQS (given we have multiple now) and make it easier to manage (we don't have to modify S3 bucket configuration directly as much if anything changes on the SQS queue side).

@philerooski
Copy link
Contributor Author

@rxu17

Were some of the main reasons for us moving from S3 -> SQS to S3 -> SNS -> SQS due to being able to decouple our S3 bucket from SQS (given we have multiple now) and make it easier to manage

No, it's not possible to have S3 send event notifications to more than one destination for the same S3 prefix. I'm not sure why this is, but maybe they don't want to have to support SNS-like capabilities for every service they implement event hooks for.

@rxu17
Copy link
Contributor

rxu17 commented Feb 22, 2024

No, it's not possible to have S3 send event notifications to more than one destination for the same S3 prefix. I'm not sure why this is, but maybe they don't want to have to support SNS-like capabilities for every service they implement event hooks for.

I thought we could configure multiple notification configs to the S3 bucket with the same s3 prefix. Guessing that doesn't execute. Bummer.

return sqs_event_record

def create_sns_notification(s3_event_record):
Copy link
Member

Choose a reason for hiding this comment

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

Nit: typing hinting

Copy link
Member

@thomasyu888 thomasyu888 left a comment

Choose a reason for hiding this comment

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

🔥 LGTM! just a tiny Nit, but going to approve.

@philerooski philerooski merged commit 26d369b into main Feb 26, 2024
15 checks passed
@philerooski philerooski deleted the etl-621 branch February 26, 2024 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants