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

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

Merged
merged 5 commits into from Apr 17, 2018

Conversation

@mashrikt
Copy link
Contributor

@mashrikt mashrikt commented Feb 13, 2018

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

@mashrikt
Copy link
Contributor Author

@mashrikt 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'
Copy link
Member

@safwanrahman safwanrahman Feb 13, 2018

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')
Copy link
Member

@safwanrahman safwanrahman Feb 13, 2018

Also this one

@safwanrahman
Copy link
Member

@safwanrahman 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
Copy link
Member

@safwanrahman safwanrahman Feb 19, 2018

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

Copy link
Member

@humitos humitos Feb 20, 2018

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.

Copy link
Member

@safwanrahman safwanrahman Feb 26, 2018

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 ?

Copy link
Contributor

@agjohnson agjohnson Feb 27, 2018

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

Copy link
Contributor

@shreyateeza shreyateeza left a comment

@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. 😅

Copy link
Contributor

@agjohnson agjohnson left a comment

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.

@mashrikt mashrikt force-pushed the master branch 2 times, most recently from 9856d31 to 3d4d4ea Mar 1, 2018
@humitos
Copy link
Member

@humitos 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
Copy link
Member

@safwanrahman safwanrahman commented Mar 1, 2018

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

@humitos
Copy link
Member

@humitos humitos commented Mar 2, 2018

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

@mashrikt
Copy link
Contributor Author

@mashrikt mashrikt commented Mar 6, 2018

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

@safwanrahman
Copy link
Member

@safwanrahman safwanrahman commented Mar 11, 2018

@agjohnson Possible to have another round of review?

@RichardLitt RichardLitt requested a review from humitos Mar 14, 2018
Copy link
Contributor

@agjohnson agjohnson left a comment

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),
Copy link
Contributor

@agjohnson agjohnson Mar 16, 2018

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
Copy link
Member

@safwanrahman 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
Copy link
Member

@safwanrahman safwanrahman commented Mar 24, 2018

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

Copy link
Member

@humitos humitos left a comment

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

@safwanrahman safwanrahman force-pushed the master branch 2 times, most recently from eca8077 to 0b711ea Mar 26, 2018
@safwanrahman
Copy link
Member

@safwanrahman safwanrahman commented Mar 26, 2018

Rebased Again and its ready to merge

@humitos humitos merged commit d458592 into readthedocs:master Apr 17, 2018
1 check passed
@RichardLitt
Copy link
Member

@RichardLitt RichardLitt commented Apr 19, 2018

Thank you @mashrikt!

@mashrikt
Copy link
Contributor Author

@mashrikt 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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants