Skip to content
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

semaphore with dynamic key+number #11890

Open
tooptoop4 opened this issue Sep 26, 2023 · 4 comments
Open

semaphore with dynamic key+number #11890

tooptoop4 opened this issue Sep 26, 2023 · 4 comments
Labels
area/mutex-semaphore area/spec Changes to the workflow specification. type/feature Feature request

Comments

@tooptoop4
Copy link
Contributor

tooptoop4 commented Sep 26, 2023

Summary

Currently mutex allows a dynamic key (example incoming tablename from an argo event). But only allows max 1 wf to consume it.
Currently semaphore requires a static key name (pre-defined in configmap) but allows n number of wfs to consume it.

This new proposal is about having the flexibility from both worlds. ie a semaphore that allows dynamic key (not pre-defined in configmap) and ability to specify n wfs to consume it.

I imagine on a sensor it could look like:

  synchronization:
    semaphore:
      name: "{{=...parse_tbl from sensor message}}"
      maxInParallel: 3

Use Cases

To dynamically rate limit the platform, avoid new tables being resource hogs

@agilgur5
Copy link
Member

agilgur5 commented Sep 30, 2023

Hmm I think this proposal has a bit of an issue -- maxInParallel is defined on one Workflow in the example, but it can actually be defined on any Workflow.

The ConfigMap for semaphores acts as a single source of truth between all Workflows that use the same semaphore.

I suppose this could be rewritten where only the number is taken from a ConfigMap (and not the key), something like customKey or overrideKey, but that is perhaps a bit awkward/unintuitive. Might be good to think of a more ergonomic API for this

@agilgur5 agilgur5 added the area/spec Changes to the workflow specification. label Sep 30, 2023
@tooptoop4
Copy link
Contributor Author

maxInParallel in this case could even be specific to the workflow for my usecase. don't mind having the wfprefix name appended to the semaphore name, the key thing being that a new key name (ie table value could appear in sensor expression) but me not having to statically define that name beforehand

@agilgur5
Copy link
Member

agilgur5 commented Oct 1, 2023

maxInParallel in this case could even be specific to the workflow for my usecase

Hmm that is a slightly different use-case. That straddles in between the current parallelism feature and semaphores.

not having to statically define that name beforehand

But yea I understand the use-case and do think it's not altogether that unique and therefore broadly useful. There are some small gaps (like this one and multi-semaphore) in the current synchronization API that some use-cases aren't quite covered by.

The ergonomics of the API and how that compares to existing APIs would be the main challenge now.
If there are multiple Workflows, then the ConfigMap I still think makes the most sense and would be most familiar.
Perhaps valueFromKey while key can be overwritten. I don't really like any of the options I proposed so far, so if you have any ideas on that, please do list them!

@agilgur5
Copy link
Member

Dynamic keys were also suggested in #7302 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/mutex-semaphore area/spec Changes to the workflow specification. type/feature Feature request
Projects
None yet
Development

No branches or pull requests

2 participants