[18.0][IMP] queue_job: Configure default subchannel capacity.

[amh-mw]

This adds a new subcapacity option to channels that allows the configuration of the default capacity for autocreated child channels.

For example, environment ODOO_QUEUE_JOB_CHANNELS=root:8:subcapacity=1 would set the capacity of an autocreated root.sub channel to 1.

[OCA-git-bot]

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

[amh-mw]

I perceive this 0 or None falsy coercion as contrived. I'm open to a better idiom.

[florentx]

Doing it during configuration parsing is an alternative. Then you can have same thing as for capacity.

Suggested change

	self.subcapacity = int(config.get("subcapacity", 0)) or None
	self.subcapacity = config.get("subcapacity", None)

--- a/queue_job/jobrunner/channels.py
+++ b/queue_job/jobrunner/channels.py
@@ -897,7 +897,16 @@ class ChannelManager:
                             f"Invalid channel config {config_string}: "
                             f"duplicate key {k}"
                         )
-                    config[k] = v
+                    if k == "subcapacity":
+                        try:
+                            config[k] = int(v)
+                        except Exception as ex:
+                            raise ValueError(
+                                f"Invalid channel config {config_string}: "
+                                f"invalid subcapacity {v}"
+                            ) from ex
+                    else:
+                        config[k] = v
             else:
                 config["capacity"] = 1
             res.append(config)

[sbidoul]

Oh I had not seen this resolved comment. I personally prefer to keep this in the configure method, if only for symmetry with how the other options are handled.

How about self.subcapacity = int(config.get("subcapacity") if "subcapacity" in config else None ?

[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]

This error handling is nice but I think it's better to move it to the configure() method.

[amh-mw]

I don't follow. This is nearly identical to the "capacity" error handling earlier in this same method.

[sbidoul]

Hm, right, so this was already messy before, with the type conversions spread between parse_simple_config() and configure() for different options.

I'd prefer to have all the type conversions in the same place, though, so let's not make it worse here? I can refactor in a followup, so as you prefer.