-
-
Notifications
You must be signed in to change notification settings - Fork 460
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
[IMP] queue_job: Add split method #658
base: 14.0
Are you sure you want to change the base?
Conversation
Hi @guewen, |
f1e4b8b
to
6131cf1
Compare
queue_job/delay.py
Outdated
""" | ||
|
||
total_records = len(self.recordset) | ||
count = 1 + total_records // size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a division without remainder, won't this return one too many?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, indeed. I have rewritten the loop and added some tests.
queue_job/delay.py
Outdated
count = 1 + total_records // size | ||
|
||
delayables = [] | ||
for batch in range(count): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps simpler to let range
do more of the work?
for start in range(0, total_records, size):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea
queue_job/delay.py
Outdated
group.delay() | ||
return [d._generated_job for d in group._delayables] | ||
|
||
delayable.delay() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? Couldn't a developer just call delayable.split()
themselves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
6131cf1
to
feab369
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The coverage does not seem to include the unittest :/ |
Import the new test in |
facdc08
to
46f344e
Compare
They weren't run as they were |
46f344e
to
3865f14
Compare
I added a chain parameter to split into a chain instead of a group. Is there anything lacking for this to be merged? |
Just the attention of @OCA/connector-maintainers |
This PR has the |
1 similar comment
This PR has the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks! Just some minor remarks :)
@guewen ping :)
# pylint: disable=odoo-addons-relative-import | ||
from odoo.addons.queue_job.delay import Delayable, DelayableGraph | ||
|
||
|
||
class TestDelayable(unittest.TestCase): | ||
class TestDelayable(common.TransactionCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no transaction required here. Use TestCase or BaseCase
from odoo.addons.queue_job.delay import Delayable | ||
|
||
|
||
class TestDelayableSplit(common.TransactionCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same regarding the base class
"""Split the Delayable into a DelayableGroup or DelayableChain | ||
if `chain` is True containing batches of size `size` | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"""Split the Delayable into a DelayableGroup or DelayableChain | |
if `chain` is True containing batches of size `size` | |
""" | |
"""Split the Delayables. | |
Use `DelayableGroup` or `DelayableChain` | |
if `chain` is True containing batches of size `size` | |
""" |
This is a draft PR trying an alternative approach for #566
The
split
function onDelayable
returns aDelayableGroup
.This is an attempt and I am open to suggestions