[FIX] queue_job: Commit was removed, #507
Conversation
|
Hi @guewen, |
pedrobaeza
left a comment
pedrobaeza
left a comment
There was a problem hiding this comment.
This is not correct. You should never commit in any regular transaction. Why is it a problem?
|
Put a limit on the search |
It was like this before and I was told it was ok to commit this way because a new cursor is already opened for a cron and the cron only delete stuff, so it seems fine to commit after the unlink : #313 (comment) On instance that creates a lot of job records each day, you may ran into timeout issues with this cron and without commit, it will just be stuck forever. With a commit of the deleted jobs, there are good chance the cron run smootly when it launches again. It seems to me that the only point to make batch of job to delete is to be able to commit from time to time. |
|
It's true that a new cursor is opened for a cron, but:
Thus, a commitment solution is to put a limit (even configurable through a system parameter if you prefer), to restrict the number of jobs to clean. |
But each cron has its own new cursor, so I don't think it is an issue if other crons have been executed before, not sure I understand your point.
Actually, the cron runs once a day, and its goal is to delete every job which are 1 month old. If we put a limit to the cron, it will only delete a part of the jobs and the number to delete will be increasing each day... Unless you run the job more often and try to configure a limit that match the number of job created each day divided by the time the cron run each day, but it seems a lot of work for nothing ! If opening one new cursor for the seach/unlink part makes it ok, I guess it can be done this way...But I really believe that committing is important for some project, and I really don't see where it would be an issue for this cron. |
|
And, it was added in a PR, respecting all the possible rules, approved by 2 PSC and then kind of silently removed in a PR which had no link with this feature (migration of another module), I think it is not the way to go... |
|
there is already a limit of 1000 jobs, the problem is that if you have to process 100.000 jobs, it will get stuck forever if, by some reason, it fails, as no records are deleted and it will restart from the begining. We might try to make 1000 just for each channel, but the same error might happen if you have a lot of channels. If that's a problem, we might try to make it using a specific transaction like it is done on https://github.com/OCA/server-tools/blob/14.0/autovacuum_message_attachment/models/autovacuum_mixin.py#L17-L35 🤔 |
|
The problem is the loop itself. Each pass of the cron should just unlink a limit of jobs. Remove the |
|
IMO there was nothing wrong on the commit. Odoo applies the same logic on several crons https://github.com/odoo/odoo/blob/f5ffcf7feec5526a483f8ddd240648c084351008/addons/crm/models/crm_lead.py#L2278 Also, it was already merged but deleted by someone on an unrelated PR. |
|
Note all the comments and tricks like |
|
Just the first one has that trick. |
|
And other needs to avoid the test mode, other makes a previous |
|
the test can be avoided using the same logic applied on 15 and 16. It was not done because this code was not tested on GH Actions, actually it should be a good practice to add the test verification 😄 |
…ou generated too much records
etobella
force-pushed
the
14.0-fix-queue
branch
from
33dd360 to
226ceee
Compare
|
Don't insist: it's a bad practice, and it can be circumvented on the ways I have already commented. Any other thing not done on this way will lead in a future to a weird behavior. It's very easy to you to fix your problem: remove the |
|
I also have this same problem, and I'd like to get it resolved as soon as possible since it just gets worse over time. I see the discussion stopped two weeks ago. @etobella are you still working on this and planning to incorporate the types of changes Pedro suggested? If you have an alternative solution already in use or simply don't have the time to work on this, I will test the other suggested solutions instead and create a new pull request with those changes. |
They do this because they execute direct SQL, it is unrelated to the commit. |
|
/ocabot merge patch |
|
This PR looks fantastic, let's merge it! |
|
Congratulations, your PR was merged at 1ab8642. Thanks a lot for contributing to OCA. ❤️ |
6 participants
That could be a problem if you generated too much records
removed on #315
Needs to be ported to 15.0 and 16.0 too. CI was fixed on 13.0