[13.0] Add ddmrp_cron_actions_as_job #81

guewen · 2020-10-29T08:09:53Z

It makes every calls to "cron_actions" run in queue jobs.

The jobs have an identity key with "identity_exact", meaning that only
one occurence of a job for the same buffer with the same arguments
(only_nfp) will be created at a time (e.g. when the state of a
stock.move is changed several times in the same transaction or in
a different transaction in a short timeframe).

It needs OCA/queue#274 and OCA/queue#275

OCA-git-bot · 2020-10-29T08:10:03Z

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

simahawk

LG

ddmrp/tests/common.py

ddmrp_cron_actions_as_job/models/stock_buffer.py

guewen · 2020-10-29T13:09:11Z

OCA/queue#270 has a bug, so I'll propose a temporary alternative solution until I can fix this one

changed implementation, and need some work from me on the queue_job PRs to be ready

guewen · 2020-11-02T07:35:00Z

I pushed a new implementation that works by patching the method instead of relying on a decorator (see OCA/queue#270 (comment)). I still need to work on OCA/queue#274 for this to be ready.

mileo · 2021-02-02T15:26:31Z

Could you add the module to oca_requirements.txt?

guewen · 2021-02-03T07:49:56Z

Could you add the module to oca_requirements.txt?

done

guewen · 2021-02-03T07:57:52Z

@JordiBForgeFlow @LoisRForgeFlow

I included the commits of #66 with a new commit fixing issues of #66:

Call commit once per chunk instead of once per record: commit flushes
and clears the env, forcing a fetch of all the records of the chunk
after each line.
Remove buffer.refresh() in cron_ddmrp(): this deprecated method
calls invalidate_cache() without ids and field names, which forces
a fetch of all the records of the chunk at each line. If it is
required to invalidate something (but I could not find any
explanation), we should invalidate only the ids and fields that needs
invalidation (hopefully only non-stored computed fields). Or at the
very least, do not fetch the 50 records of the browse at once but one
by one (but that'd be a pity).

The duration of cron_ddmrp() (when generating jobs with ddmrp_cron_actions_as_job) goes from 8 minutes to 35 seconds.

Pay attention to the usage of refresh() / invalidate_cache() without specifying what to invalidate. It will force fetching again the records and computations, for all the pre-fetched ids. In the case of cron_actions, forcing to read the whole chunk again. As there was no indication why it was necessary, I removed it (when delaying a queue job it's never needed actually).

guewen · 2021-02-03T08:28:41Z

Pay attention to the usage of refresh() / invalidate_cache() without specifying what to invalidate. It will force fetching again the records and computations, for all the pre-fetched ids. In the case of cron_actions, forcing to read the whole chunk again. As there was no indication why it was necessary, I removed it (when delaying a queue job it's never needed actually).

As some tests failed, I looked again at this and found some fields to invalidate: ce5d553, these are the non-stored computed fields used by cron_actions

It makes calls to "cron_actions" run in queue jobs. The jobs have an identity key with "identity_exact", meaning that only one occurence of a job for the same buffer with the same arguments (only_nfp) will be created at a time (e.g. when the state of a stock.move is changed several times in the same transaction or in a different transaction in a short timeframe). It needs OCA/queue#274 and OCA/queue#275

commits and locking the buffers for too long.

* Call commit once per chunk instead of once per record: commit flushes and clears the env, forcing a fetch of all the records of the chunk after each line. * Remove `buffer.refresh()` in cron_ddmrp(): this deprecated method calls `invalidate_cache()` without ids and field names, which forces a fetch of all the records of the chunk at each line. If it is required to invalidate something (but I could not find any explanation), we should invalidate only the ids and fields that needs invalidation (hopefully only non-stored computed fields). Or at the very least, do not fetch the 50 records of the browse at once but one by one (but that'd be a pity). The duration of cron_ddmrp() (when generating jobs with ddmrp_cron_actions_as_job) goes from 8 minutes to 35 seconds.

Fix tests: * test_22_procure_recommended * test_23_buffer_zones_all It is possible that the invalidation is only required because of the tests and would not be required at runtime. But this has been proposed in the past: OCA#59 However, this commit tries to invalidate the less possible fields to limit the computations and data to fetch again.

jgrandguillaume

LGTM for what I can judge about.

LoisRForgeFlow

LGTM 👍

Thanks for the detailed commit messages, it eases following the rational behind the changes.

ddmrp/models/stock_buffer.py

OCA-git-bot · 2021-02-03T11:35:37Z

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). 🤖

LoisRForgeFlow · 2021-02-03T11:35:55Z

/ocabot merge minor

OCA-git-bot · 2021-02-03T11:36:21Z

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 13.0-ocabot-merge-pr-81-by-LoisRForgeFlow-bump-minor, awaiting test results.

OCA-git-bot · 2021-02-03T12:31:07Z

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

guewen changed the title ~~Add ddmrp_cron_actions_as_job~~ [13.0] Add ddmrp_cron_actions_as_job Oct 29, 2020

simahawk previously approved these changes Oct 29, 2020

View reviewed changes

ddmrp/tests/common.py Show resolved Hide resolved

sebalix reviewed Oct 29, 2020

View reviewed changes

ddmrp_cron_actions_as_job/models/stock_buffer.py Outdated Show resolved Hide resolved

guewen added the needs fixing label Oct 29, 2020

guewen changed the title ~~[13.0] Add ddmrp_cron_actions_as_job~~ [WIP][13.0] Add ddmrp_cron_actions_as_job Oct 29, 2020

guewen force-pushed the 13.0-ddmrp-auto-nfp-job branch from 24919eb to 50239d7 Compare November 2, 2020 07:29

guewen force-pushed the 13.0-ddmrp-auto-nfp-job branch 2 times, most recently from 83f87dc to 5515cf3 Compare November 2, 2020 15:41

guewen changed the title ~~[WIP][13.0] Add ddmrp_cron_actions_as_job~~ [13.0] Add ddmrp_cron_actions_as_job Nov 2, 2020

guewen force-pushed the 13.0-ddmrp-auto-nfp-job branch from 5515cf3 to b95bb31 Compare November 2, 2020 20:29

guewen removed the needs fixing label Nov 3, 2020

guewen force-pushed the 13.0-ddmrp-auto-nfp-job branch from b95bb31 to 1c31e05 Compare February 2, 2021 15:23

guewen force-pushed the 13.0-ddmrp-auto-nfp-job branch from 1c31e05 to 5c81d21 Compare February 3, 2021 07:49

guewen force-pushed the 13.0-ddmrp-auto-nfp-job branch from 5c81d21 to a49002f Compare February 3, 2021 07:53

guewen and others added 5 commits February 3, 2021 09:54

use split_every in buffer crons in order to allow more frequent

8123cb4

commits and locking the buffers for too long.

commit only if we are not testing

c4ae03e

guewen force-pushed the 13.0-ddmrp-auto-nfp-job branch from ce5d553 to 92ae390 Compare February 3, 2021 08:54

guewen mentioned this pull request Feb 3, 2021

[13.0][ddmrp] use split_every in buffer crons in order to allow more frequent commits #66

Closed

jgrandguillaume approved these changes Feb 3, 2021

View reviewed changes

LoisRForgeFlow approved these changes Feb 3, 2021

View reviewed changes

ddmrp/models/stock_buffer.py Show resolved Hide resolved

OCA-git-bot added approved ready to merge labels Feb 3, 2021

OCA-git-bot added the bot is merging ⏳ label Feb 3, 2021

LoisRForgeFlow mentioned this pull request Feb 3, 2021

refresh buffer during cron action #59

Closed

OCA-git-bot merged commit 33efc3a into OCA:13.0 Feb 3, 2021

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Feb 3, 2021

nguyenhk99 mentioned this pull request Nov 16, 2021

[14.0][FW] ddmrp: multiple ports from 13.0 #179

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[13.0] Add ddmrp_cron_actions_as_job #81

[13.0] Add ddmrp_cron_actions_as_job #81

guewen commented Oct 29, 2020 •

edited

Loading

OCA-git-bot commented Oct 29, 2020

simahawk left a comment

guewen commented Oct 29, 2020

guewen commented Nov 2, 2020

mileo commented Feb 2, 2021

guewen commented Feb 3, 2021

guewen commented Feb 3, 2021 •

edited

Loading

guewen commented Feb 3, 2021

jgrandguillaume left a comment

LoisRForgeFlow left a comment

OCA-git-bot commented Feb 3, 2021

LoisRForgeFlow commented Feb 3, 2021

OCA-git-bot commented Feb 3, 2021

OCA-git-bot commented Feb 3, 2021

[13.0] Add ddmrp_cron_actions_as_job #81

[13.0] Add ddmrp_cron_actions_as_job #81

Conversation

guewen commented Oct 29, 2020 • edited Loading

OCA-git-bot commented Oct 29, 2020

simahawk left a comment

Choose a reason for hiding this comment

guewen commented Oct 29, 2020

guewen commented Nov 2, 2020

mileo commented Feb 2, 2021

guewen commented Feb 3, 2021

guewen commented Feb 3, 2021 • edited Loading

guewen commented Feb 3, 2021

jgrandguillaume left a comment

Choose a reason for hiding this comment

LoisRForgeFlow left a comment

Choose a reason for hiding this comment

OCA-git-bot commented Feb 3, 2021

LoisRForgeFlow commented Feb 3, 2021

OCA-git-bot commented Feb 3, 2021

OCA-git-bot commented Feb 3, 2021

guewen commented Oct 29, 2020 •

edited

Loading

guewen commented Feb 3, 2021 •

edited

Loading