OCA / queue

Asynchronous Job Queue
GNU Affero General Public License v3.0
179 stars 451 forks source link

[16.0] [ENH] queue_job: identity_key enhancements #546

Closed richard-willdooit closed 9 months ago

richard-willdooit commented 1 year ago
  1. In production, a job which is waiting dependencies or which has started, but not completed, should not be repeated if the identity_key matches.
  2. In tests, the mock queue handler is now enhanced to allow better mimicking of the identity_key blocks from production.
  3. In tests, the mock queue handler now clears the enqueued jobs after performing them, to better reproduce what a production environment would do.
OCA-git-bot commented 1 year ago

Hi @guewen, some modules you are maintaining are being modified, check this out!

richard-willdooit commented 1 year ago

@guewen

Another option I was thinking about, was the option to, for certain jobs, not allow them to be requeued from the queue.

In our scenario, we kicked off the job from a button. But if the job fails, and then the data changes in such a way that the button is no longer applicable, then the job has lost applicability, too - and should not be requeued - I have done the right thing and made sure our method is autonomous in its own right, and checks that the method is still applicable, but I did wonder if there was a place for special jobs which should be tried once, and once only, and could not be requeued even if they failed...

Ultimately, the main changes I wanted were to introduce tests in our module that replicated the behaviour of our process not allowing the double queuing of the process.

Oh to give even more context to how we have used it:

If I call write to 2 res company records with 4 values, then 3 values are written to both records, and 2 jobs are enqueued to update the other value on the 2 records. It works well, but tests were quite important to show that the behaviour of this was as expected, and then I decided to extend to count the jobs queued to show that double queuing wold not happen for the same record - which it did not in production, but the mock test object incorrectly indicated it had.

richard-willdooit commented 1 year ago

@guewen

If you would like me to remove the states from the PR, and just push the test changes, I can do so - or you can modify my PR as you see fit. We would likely then work from my fork for our environment. Let me know.

guewen commented 1 year ago

@richard-willdooit

If you would like me to remove the states from the PR, and just push the test changes, I can do so - or you can modify my PR as you see fit. We would likely then work from my fork for our environment. Let me know.

I think you may remove the STARTED state, the WAIT_DEPENDENCIES one seems good to me. So we can merge this first part. Then up to you if you want to revise the STARTED use case in a new PR.

simahawk commented 1 year ago

@richard-willdooit thanks for this nice work :) I agree w/ @guewen on removing the STARTED state check for now. Then we can merge.... I'm eager to test it on v14 :wink:

simahawk commented 1 year ago

@richard-willdooit one more thing: when you get back to this, please remove the odoo version from the commit. Is not needed and it makes little sense when we fwd/bkport changes. Thanks!

simahawk commented 11 months ago

@richard-willdooit gentle ping: any plan to move this fwd?

simahawk commented 9 months ago

As per #581 I'm taking over on #587