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

✨(mailbox_manager) add shedule task to fetch domains status #662

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sdemagny
Copy link
Contributor

@sdemagny sdemagny commented Jan 25, 2025

Add celery crontab to check and update domains status. This task calls dimail API.

TODO:

  • improve task return to give more information
  • add notification (here)
  • add flower to helm chart

@sdemagny sdemagny force-pushed the sdem/add_celery_task_fetch_domain_status branch 2 times, most recently from 248536a to ecf65c0 Compare January 26, 2025 13:53
@sdemagny sdemagny force-pushed the sdem/add_celery_task_fetch_domain_status branch 5 times, most recently from 4170d68 to 5bfccf9 Compare January 27, 2025 00:16
@sdemagny sdemagny self-assigned this Jan 27, 2025
@sdemagny sdemagny force-pushed the sdem/add_celery_task_fetch_domain_status branch 4 times, most recently from 099f029 to b74aa7a Compare January 27, 2025 16:51
@sdemagny sdemagny marked this pull request as ready for review January 27, 2025 16:51
@sdemagny sdemagny requested a review from qbey January 28, 2025 09:12
Copy link
Member

@qbey qbey left a comment

Choose a reason for hiding this comment

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

Did you consider adding the sentry for celery? https://docs.sentry.io/platforms/python/integrations/celery/

@sdemagny sdemagny force-pushed the sdem/add_celery_task_fetch_domain_status branch 2 times, most recently from f0081d2 to 4b3bb9f Compare February 3, 2025 09:08
Copy link
Contributor

@Morendil Morendil left a comment

Choose a reason for hiding this comment

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

When trying this locally, a file src/backend/celerybeat-schedule is created - it would be good to add it to .gitignore ?

else:
check_count += 1
# todo: add more details to store more information in the database
return f"Domains processed: {update_count} updated, {check_count} checked"
Copy link
Contributor

Choose a reason for hiding this comment

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

The counting logic is counter-intuitive. A domain is counted as either checked or updated (so you can have 1 updated and 0 checked). What I would expect is that "updated" is a subset of "checked"…

Also, domains that failed verification are not counted at all… and we probably want to report them because the failure logs are going to be hidden in the Celery container's logs, not directly visible here.

I would prefer something like

f"Domains processed: {same_count} not changed, {update_count} updated, {failure_count} failed, {check_count} checked"

with the invariant same_count+update_count+failure_count == check_count.

Add new container to run celery beat to manage schedule job
@sdemagny sdemagny force-pushed the sdem/add_celery_task_fetch_domain_status branch from 4b3bb9f to 736595b Compare February 27, 2025 02:01
Add celery crontab to check and update domains status.
This task calls dimail API.
Allow to manage and monitor celery tasks
This allow to start a celery worker and a celery beat
@sdemagny sdemagny force-pushed the sdem/add_celery_task_fetch_domain_status branch from 736595b to c229a8a Compare February 27, 2025 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants