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

Escape refresh job for already fresh cache #95

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

walison17
Copy link
Contributor

Add a should_refresh method to escape async refresh when the cache is already fresh. This can happen when the worker is slow and there are too many refresh jobs to process.

@walison17 walison17 changed the title Escape refresh for already fresh jobs Escape refresh job for already fresh cache Oct 25, 2021
@walison17 walison17 force-pushed the refresh-escape branch 2 times, most recently from 987d6e8 to eabb15d Compare October 25, 2021 13:22
@mock.patch('tests.test_base_job.EmptyDummyJob.fetch', return_value='foo')
def test_escape_job_refresh(self, fetch_mock, rq_burst):
job = EmptyDummyJob()
job.task_options = {'is_async': False}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this option for?

Copy link
Contributor Author

@walison17 walison17 Oct 27, 2021

Choose a reason for hiding this comment

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

by default (is_async = True) jobs will be consumed in different threads and call_count don't work properly with many threads

@avelino
Copy link

avelino commented Jan 13, 2025

@stephrdev is there anything missing for this PR to be accepted?

@stephrdev
Copy link
Collaborator

@stephrdev is there anything missing for this PR to be accepted?

I think I totally missed that one. After looking at it again I think this could be merged if the feature is configurable. Don't want to break things that rely on refreshing "newer" values.

@walison17
Copy link
Contributor Author

@stephrdev is there anything missing for this PR to be accepted?

I think I totally missed that one. After looking at it again I think this could be merged if the feature is configurable. Don't want to break things that rely on refreshing "newer" values.

I can make this feature configurable by introducing a setting called CACHEBACK_TASK_ESCAPE_ALREADY_FRESH. It would default to false to ensure backward compatibility.

@stephrdev
Copy link
Collaborator

@stephrdev is there anything missing for this PR to be accepted?

I think I totally missed that one. After looking at it again I think this could be merged if the feature is configurable. Don't want to break things that rely on refreshing "newer" values.

I can make this feature configurable by introducing a setting called CACHEBACK_TASK_ESCAPE_ALREADY_FRESH. It would default to false to ensure backward compatibility.

Sound great, thanks! I thought about the setting name and would propose CACHEBACK_VALIDATE_JOB_REFRESH_NEEDED in addition to some explaining docs. 👍

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.

3 participants