[18.0][IMP] queue_job: Default subchannel capacity and sequential.

[amh-mw]

This adds capacity_default and sequential_default options that set capacity and sequential for autocreated subchannels. It also allows non-root channel configurations to omit capacity.

[OCA-git-bot]

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

[sbidoul]

This looks good, thanks!

Could expand the docstring of parse_simple_config a bit (around line 823).

The only question I'd have is if we can come up for a better name to convey "default subchannel capacity". I don't have any better idea for now.

[amh-mw]

The only question I'd have is if we can come up for a better name to convey "default subchannel capacity". I don't have any better idea for now.

Given existing documentation:

Channels: give a capacity for the root channel and its sub-channels and segregate jobs in them. Allow for instance to restrict heavy jobs to be executed one at a time while little ones are executed 4 at a times.

Noodling around in tests:

cm=channels.ChannelManager()
cm.simple_configure("root:2,child")
root=cm.get_channel_by_name("root")
self.assertEqual(root.capacity, 2)
child=cm.get_channel_by_name("child")
self.assertEqual(child.capacity, 1)
auto=cm.get_channel_by_name("auto", autocreate=True)
self.assertEqual(auto.capacity, None)

The difference in capacity between an autocreated subchannel and a configured subchannel was the impetus for the creation of this pull request. Maybe rename subcapacity to autocapacity? Testing shows that the new setting does not change the capacity for configured subchannels.

cm=channels.ChannelManager()
cm.simple_configure("root:4:subcapacity=2,child")
root=cm.get_channel_by_name("root")
self.assertEqual(root.capacity, 4)
child=cm.get_channel_by_name("child")
self.assertEqual(child.capacity, 1)

[github-actions]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

[VecSec]

Hi, awesome feature great job. May you merge? @amh-mw

[sbidoul]

I still don't have a better idea for a config name. default_subchannel_capacity might be better but it's a bit long?

Can you add doctests in the ChannelManager class? You can look at how it is done for the throttle config to get inspiration.

[sbidoul]

While discussing this with a colleague we found a use case where it would be interesting to configure subchannels with default capacity 1 + sequential, so maybe we should generalize this PR.

[amh-mw]

While discussing this with a colleague we found a use case where it would be interesting to configure subchannels with default capacity 1 + sequential, so maybe we should generalize this PR.

@sbidoul If you would be so kind as to create a new issue for that use case, I'd be happy to take a look, but I am loathe to add features to an approved pull request that is about to turn one year old. 🎉

[sbidoul]

Hi @amh-mw, I've looked at this again and I think my last comments still stand.

• I would like the test to be added as doctests to ChannelManager.simple_configure, like other similar tests, instead of new test classes. I'm aware the doctests currently don't run in CI but they can be run manually, and it's important to keep similar things together.
• Can you please move the parsing of the subcapacity config value to the configure method, like it is done for throttle, to keep the same code style. Except for capacity which is special for the config parser, the parse_simple_config method does not have specific knowledge of the config keys.
• Since we will likely want to extend to other default config keys for subchannels, I propose to rename it to def_sub_capacity. So we can later add def_sub_sequential, etc.

[amh-mw]

While discussing this with a colleague we found a use case where it would be interesting to configure subchannels with default capacity 1 + sequential, so maybe we should generalize this PR.

I went ahead and added sequential_default.

[OCA-git-bot]

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

[sbidoul]

Thanks! This is a useful feature.

[amh-mw]

Hold off for a second, test logs have

**********************************************************************
File "/mnt/extra-addons/oca/queue/queue_job/jobrunner/channels.py", line 986, in odoo.addons.queue_job.jobrunner.channels.ChannelManager.simple_configure
Failed example:
cm.get_channel_by_name('root.seq.auto', autocreate=True).capacity
Expected:
1
Got nothing
**********************************************************************
1 items had failures:
1 of 23 in odoo.addons.queue_job.jobrunner.channels.ChannelManager.simple_configure
***Test Failed*** 1 failures.

but no error.

[amh-mw]

I removed the defaulting of capacity=1 when sequential=True and gave doctest its teeth back.