Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Issue1248: Persistent Email Reminders #1354
Issue1248: Persistent Email Reminders #1354
Changes from 15 commits
5cd5119
b752ba3
ce3b56d
3f4340e
32bea9d
382a924
653a480
93b7f9b
b73d12a
e8dd359
04994b5
fa5e7df
be83075
b5379aa
79c5c5b
3a669ba
e567009
0468acd
5e0d8e4
2b9bdf9
9aaeb13
efa27ce
6eeaf93
950ae28
87981e7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to review the design a bit because this pattern is new to data.all:
At first sight it seems like we can simplify the workflow and run everything from ECS. Instead of creating Worker tasks (lines 235-249) we could directly call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@noah-paige @petrkalos @SofiaSazonova I would like your opinion on this one :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only benefit of sending task to queue for worker lambda is if the worker lambda has code bundled in its image that is not there for the ECS container. However this is not the case and both images should have all the backend code at their disposal for whatever data.all task
While separating out all of our backend code into a more microservice aligned design could be useful - it is not in scope for this PR and requires much more design consideration and careful thought
TLDR - I think let's just keep it all in ECS :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually had discussed the design with Noah before implementing it and we considered both options:
Option 1: Create a new ECS task that finds pending shares, creates tasks for them, and queues them up, then uses the existing worker Lambda to send emails.
Option 2: Use the send_email_task directly, as you suggested.
I chose Option 1 because I thought having a queue would be beneficial for managing a high volume of outgoing emails. It helps to queue up tasks and avoid potential clashes.
I am open to hearing what others have to say too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to vote for option 1: If we were implementing this from scratch, I would go for a queue + Lambda to process the emails. The part that I do not like form the current architecture is that we are using an ECS task to do very minor things. The new pattern that I would like to see is: scheduled Lambda ---> SQS queue ---> Worker Lambda. But that is out of scope for this PR. We can approve the current scheduled ECS ---> SQS queue ---> Worker Lambda and think about scheduled ECS tasks that could be handled in Lambda in a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 after some more thought I think we maybe should just keep all compute for email reminders in ECS for now
I think it should only require a small change to L238 of
share_notification_service
to not useWorker.queue()
and instead directly callI think it could be worthwhile to keep Task creation nonetheless for auditability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@noah-paige Added
directly to
share_notification_service
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: I voted for option 1, but I am not strong opinionated.