Skip to content

Add delete stats to "backend-cron"#156

Merged
davidor merged 3 commits intomasterfrom
add-delete-stats-to-cron
Feb 3, 2020
Merged

Add delete stats to "backend-cron"#156
davidor merged 3 commits intomasterfrom
add-delete-stats-to-cron

Conversation

@davidor
Copy link
Copy Markdown
Contributor

@davidor davidor commented Feb 3, 2020

What the title says.

@davidor davidor requested a review from unleashed February 3, 2020 15:46
Copy link
Copy Markdown
Contributor

@unleashed unleashed left a comment

Choose a reason for hiding this comment

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

👍 but check comment.

Comment thread openshift/backend-cron
Comment on lines +11 to +12
def run_rake_task_in_thread(name, freq, check_once = false)
Thread.new do
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is nothing shared between the threads. Have you considered doing a fork + exec? It would avoid the pitfalls of GIL, although this seems like not much of an opportunity to block.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, there's not much blocking. Delete stats has sleeps between SCAN calls and the rescheduling only runs a few redis commands every 5 min by default.

@davidor davidor force-pushed the add-delete-stats-to-cron branch from eefaf29 to 25dcfb6 Compare February 3, 2020 16:32
@davidor
Copy link
Copy Markdown
Contributor Author

davidor commented Feb 3, 2020

@unleashed I force-pushed a minor change to keep the task under the stats namespace. I think it's more clear this way.

@unleashed
Copy link
Copy Markdown
Contributor

LGTM

@davidor davidor merged commit f1f4561 into master Feb 3, 2020
@bors bors Bot deleted the add-delete-stats-to-cron branch February 3, 2020 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants