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

[#2967] Scheduled tasks for cleaning up messages #3604

Merged
merged 5 commits into from Apr 17, 2018

Conversation

Projects
None yet
6 participants
@mashrikt
Contributor

mashrikt commented Feb 13, 2018

Set up a scheduled task every Sunday night at 23:59 to delete all expired Persistent Messages.

@mashrikt

This comment has been minimized.

Contributor

mashrikt commented Feb 13, 2018

@safwanrahman Can you please take a look?

@safwanrahman safwanrahman requested a review from agjohnson Feb 13, 2018

@@ -31,7 +31,7 @@ def DATABASES(self): # noqa
SLUMBER_API_HOST = 'http://localhost:8000'
PUBLIC_API_URL = 'http://localhost:8000'
BROKER_URL = 'redis://localhost:6379/0'
CELERY_BROKER_URL = 'redis://localhost:6379/0'

This comment has been minimized.

@safwanrahman

safwanrahman Feb 13, 2018

Member

So I think this will fail the tests!
Can you remove this and keep it like previous?

def create_application():
"""Create a Celery application using Django settings"""
os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'readthedocs.settings.dev')
application = Celery('readthedocs')
application.config_from_object('django.conf:settings')
application.config_from_object('django.conf:settings', namespace='CELERY')

This comment has been minimized.

@safwanrahman

safwanrahman Feb 13, 2018

Member

Also this one

@safwanrahman

This comment has been minimized.

Member

safwanrahman commented Feb 19, 2018

@mashrikt Can you please fix the linter?

@@ -17,3 +18,14 @@ def create_application():
app = create_application() # pylint: disable=invalid-name
@app.on_after_finalize.connect

This comment has been minimized.

@safwanrahman

safwanrahman Feb 19, 2018

Member

@agjohnson Do you have any conern about using this approach to register beat task?

This comment has been minimized.

@humitos

humitos Feb 20, 2018

Member

As far as I know, we are not using this approach but registering the beat task from a setting in the operations repository (private).

Example:

    CELERYBEAT_SCHEDULE = {
        'quarter-finish-inactive-builds': {
            'task': 'readthedocs.projects.tasks.finish_inactive_builds',
            'schedule': crontab(minute='*/15'),
            'options': {'queue': 'web'},
        },
    }

So, I suppose that we prefer that way as a general rule.

This comment has been minimized.

@safwanrahman

safwanrahman Feb 26, 2018

Member

I think its better to have this in our code rather than the private operation repository. So other users of Read the Docs platform can be benifical from it. What do you think @humitos ?

This comment has been minimized.

@agjohnson

agjohnson Feb 27, 2018

Contributor

This should be managed via settings in this repository, not in code. We have some we add in ops, but that's different.

@shreyateeza

@mashrikt Squash your commits into one. It would be much better. And if they are of different issues, then create separate branches for them.
Also it is not a good practice to send a PR directly from your master branch. 😅

@agjohnson

Move celery beat tasks to our settings, it shouldn't be done with a separate pattern. Lint errors should go away after removing this block.

@humitos

This comment has been minimized.

Member

humitos commented Mar 1, 2018

I'm sure that I left at least one comment in this PR regarding how to configure celery beat but it dissapeared :/

Anyway, @agjohnson I was saying that CELERY_BEAT setting will be overriden by our provisioning scripts, right? So, I'm not following you why you suggested him to write that setting.

The idea is to be able to have an hybrid? I mean, be able to set some task from this repo itself and then update this setting from the provisioning script?

@safwanrahman

This comment has been minimized.

Member

safwanrahman commented Mar 1, 2018

@humitos do you mean this comment? #3604 (comment)

@humitos

This comment has been minimized.

Member

humitos commented Mar 2, 2018

@safwanrahman yeah, that one. I don't know why I lost it :) Thanks.

@mashrikt

This comment has been minimized.

Contributor

mashrikt commented Mar 6, 2018

Can anyone give me some guidelines to what I should do? I am a bit lost. @safwanrahman @agjohnson

@safwanrahman

This comment has been minimized.

Member

safwanrahman commented Mar 11, 2018

@agjohnson Possible to have another round of review?

@RichardLitt RichardLitt requested a review from humitos Mar 14, 2018

@agjohnson

Looks good, but noted a change to the schedule

CELERYBEAT_SCHEDULE = {
'weekly-clear-persistent-messages': {
'task': 'readthedocs.core.tasks.clear_persistent_messages',
'schedule': crontab(hour=23, minute=59, day_of_week=6),

This comment has been minimized.

@agjohnson

agjohnson Mar 16, 2018

Contributor

Any reason this can't run on a more frequent schedule? I'd like this to run in my development instance as well, and I'm sure not going to wait up for it to run :)

Running on a hour='*/3' or something might make more sense.

@safwanrahman

This comment has been minimized.

Member

safwanrahman commented Mar 23, 2018

@mashrikt While you were working on the PR, another PR merged that added CELERYBEAT_SCHEDULE in the settings.
Is it possible for you to rebase upon master and add your task to the existing CELERYBEAT_SCHEDULE settings?
If it seems overchallenged, let me know. I will do it and push it in the PR.

@safwanrahman

This comment has been minimized.

Member

safwanrahman commented Mar 24, 2018

@agjohnson Looks good to me as well as you.
I have fixed the linter issue.
Possible to merge?

@humitos

It's ready to merge from my point of view.

@safwanrahman

This comment has been minimized.

Member

safwanrahman commented Mar 26, 2018

Rebased Again and its ready to merge

@humitos humitos merged commit d458592 into rtfd:master Apr 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@RichardLitt

This comment has been minimized.

Member

RichardLitt commented Apr 19, 2018

Thank you @mashrikt!

@mashrikt

This comment has been minimized.

Contributor

mashrikt commented Apr 19, 2018

Thanks!
First of many, hopefully!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment