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

Replace custom task cancellation with RQ 1.7 functionality #1040

Merged
merged 1 commit into from Dec 1, 2020

Conversation

dralley
Copy link
Contributor

@dralley dralley commented Nov 30, 2020

@dralley
Copy link
Contributor Author

dralley commented Nov 30, 2020

RQ feature: rq/rq#1376

@pulpbot
Copy link
Member

pulpbot commented Nov 30, 2020

Attached issue: https://pulp.plan.io/issues/7902

Copy link
Contributor

@daviddavis daviddavis left a comment

Choose a reason for hiding this comment

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

This is awesome. Good find @dralley.


resource_job.delete()
job.delete()

# A hack to ensure that we aren't deleting resources still being used by the workhorse
time.sleep(1.5)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should keep this but reduce it to ~0.1 seconds?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I'm not entirely sure what to do. I'm trying to think through it; so the resources delete while the task is in use, and it receives a database write error in the task at some point?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe that's ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's an inherent delay in between when the cancellation is triggered and when it actually happens. Before that delay was due to polling, now it's message passing so it should be faster but technically still present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main thing is that we should avoid cancellations being marked as failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bmbouter I'm thinking I should just err towards caution and add it back for now.

if resource_job.is_started:
redis_conn.sadd(TASKING_CONSTANTS.KILL_KEY, resource_job.get_id())
# A hack to ensure that we aren't deleting resources still being used by the workhorse
time.sleep(0.5)
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

Thank you for switching us back to the upstream feature.

@dralley dralley merged commit 5f7dc90 into pulp:master Dec 1, 2020
@dralley dralley deleted the rq-native branch December 1, 2020 19:41
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.

None yet

4 participants