-
Notifications
You must be signed in to change notification settings - Fork 0
Add email client to send emails via celery tasks #33
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
base: feat/mailpit-integration
Are you sure you want to change the base?
Add email client to send emails via celery tasks #33
Conversation
| except ExternalProviderException as exc: | ||
| if email.context: | ||
| countdown_in_seconds = email.context.backoff_in_seconds * ( | ||
| 2**self.request.retries |
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.
Why X 2? If necessary, maybe can be put in a constant to avoid the magic number
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.
This is a loose implementation of the retry_backoff algorithm that was used in the previous definition of the task. It's not exactly the same because I did not implement neither the jitter randomization nor the backoff max value, but the main feature, the exponential growth, is there.
2 ** self.request.retries is a power of 2 that represents the exponential growth of backoff_in_seconds. So the first time it is retried, it will wait 1x the backoff value, the second time 2x, the third time 4x, and so on.
I could define the 2 as a constant like BACKOFF_EXPONENTIAL_GROWTH_BASE (or something like that) at the top of the file, and here the code would be left as:
| 2**self.request.retries | |
| multiplicator = BACKOFF_EXPONENTIAL_GROWTH_BASE ** self.request.retries | |
| countdown_in_seconds = email.context.backoff_in_seconds * multiplicator |
Do you approve of this change? It seemed kinda overkill to me at first, but after reviewing it I think it certainly is easier to understand this way.
| from app.emails.schema.email import Email, EmailContext | ||
|
|
||
|
|
||
| class CeleryTaskEmailClient(BaseEmailClient): |
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.
It makes me noice the celery name in here. I think client shouldn't know about Celery directly in here, since i can use SES with celery in theory.
We are mixing the domain/application layer with a specific infrastructure tool (Celery).
Maybe abstracting the name to decouple implementation?
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 understand the concern.
The separation between layers is here, because the application layer should always use the EmailService, not an EmailClient instance directly, and the EmailService uses by default the email client that was configured during app initialization (although injection is possible).
For the example you mentioned (using SES with celery):
- the app would be configured to use the CeleryEmailClient, and the celery worker to use the SESClient,
- the application would use the EmailService to send emails,
- the EmailService would use the configured CeleryEmailClient to enqueue emails via celery,
- the celery worker would use the configured SESClient (or similar class) to send the emails via SES.
The configuration happens in main.py, where the set_client call is made to set up the default client for each container/environment.
So, the layers are decoupled 😄
Uh oh!
There was an error while loading. Please reload this page.