Skip to content

Conversation

@florian-dacosta
Copy link
Contributor

I ran into an issue several times (on version 8 but no reason it will be different on version 14) where the AutoVacuum Job Queue cron fails due to timeout.
As a result it starts again, fail again, and have to be unblocked manually (by deleting some jobs by script for instance)

After some digging, it seems some people had this kind of issue also in past version :
#115
and was even merged in v7 : OCA/connector#158

So I'd like to introduce the same in v14, commit every 1000 deleted jobs. (the 1000 is arbitrary, we could change it if needed, but I don't really see why it should be configurable.

I think it is important because depending on the project, the jobs may take more or less time to delete, for instance, if some mail.messages are added, it takes more time, if attachments are added (base_async_import), it takes more time, etc...
And depending how intensively jobs are used of course.

What do you think @guewen @rousseldenis @lmignon ?

)
if jobs:
jobs.unlink()
with api.Environment.manage():
Copy link
Contributor

Choose a reason for hiding this comment

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

@florian-dacosta Why do you need to use a new cursor?

@guewen
Copy link
Member

guewen commented Feb 10, 2021

On the principle yes, on the implementation, as @lmignon noted, you shouldn't need a new cursor, and as the db snapshot can change after a commit, it may be better to do a search after every batch (with a limit).
You may want to have a look at #302 too which means there is less to delete on the vacuum.

Huge amount of jobs to delete may take a lot of time and the cron may crash, because of timeout for instance
This issue would make the cron permanently run and permanently failing... Deleting batch allow to avoid this issue
if the cron fails, it still will have delete some of the job history
@florian-dacosta
Copy link
Contributor Author

I thought it was always better to do explicit commit in new cursor.
Mainly in cron, I thought it would release the lock on the cron, I am convinced I saw this kind of thing a long time ago.
But wrong belief I guess, I have adapted the PR, thanks @lmignon @guewen

You may want to have a look at #302 too which means there is less to delete on the vacuum.

Nice addition, I had not see it, but I've also done it in this v8 project, mainly for the gain at deletion indeed.
Good news it is in base module now though!

@guewen
Copy link
Member

guewen commented Feb 10, 2021

Mainly in cron, I thought it would release the lock on the cron, I am convinced I saw this kind of thing a long time ago.

The cron are already executed in a second cursor

@guewen
Copy link
Member

guewen commented Feb 10, 2021

Thanks to you :)

Copy link
Contributor

@lmignon lmignon 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 @florian-dacosta. LGTM (code review)

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@lmignon
Copy link
Contributor

lmignon commented Feb 15, 2021

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-313-by-lmignon-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit c07f6a0 into OCA:14.0 Feb 15, 2021
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 470d6b2. Thanks a lot for contributing to OCA. ❤️

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants