Skip to content

Conversation

hchepey-clari
Copy link
Contributor

@hchepey-clari hchepey-clari commented Aug 8, 2025

Pull Request type

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • WHOSUSING.md
  • Other (please describe):

NOTE: Please remember to run ./gradlew spotlessApply to fix any format violations.

Changes in this PR

Implemented a runtime-configurable rate limiting feature that allows per-workflow-instance rate limit customization while maintaining full backward compatibility.

Describe the new behavior from this PR, and why it's needed
Issue #

We have a requirement where we want to rate limit the tasks as per the different client rate limit applicable. That is dynamic based on the Workflow Inputs supplied to the Workflow.

When starting a workflow, users can provide optional taskRateLimitOverrides in the request
These overrides are keyed by reference task name (most specific) or task definition name
Task mappers check for overrides before falling back to static TaskDef values
The existing Redis rate limiting infrastructure works unchanged with the dynamic values

Alternatives considered

Describe alternative implementation you have considered

@v1r3n
Copy link
Collaborator

v1r3n commented Aug 9, 2025

Have you considered adding TaskDef with rate limits in the workflow definition when starting a workflow?

@hchepey-clari
Copy link
Contributor Author

Have you considered adding TaskDef with rate limits in the workflow definition when starting a workflow?

I just checked. Thanks for pointing that out. Let me think about it.

One thing for sure, that would mean I have to pass the entire workflowdef json in every StartWorkflow Request. And we have to operate on a scale where we have to invoke thousands of workflows every hour at a regular cadence.

@hchepey-clari
Copy link
Contributor Author

Have you considered adding TaskDef with rate limits in the workflow definition when starting a workflow?

I just checked. Thanks for pointing that out. Let me think about it.

One thing for sure, that would mean I have to pass the entire workflowdef json in every StartWorkflow Request. And we have to operate on a scale where we have to invoke thousands of workflows every hour at a regular cadence.

This change is backward compatible and would not need to pass the WorkflowDef everytime in the StartWorkflowRequest payload. We will be triggering millions of workflows everyday. Do you foresee any other concern with this change, @v1r3n ?

@hchepey-clari
Copy link
Contributor Author

@v1r3n @manan164 - Bumping up in case if this slipped off your radar.

@v1r3n
Copy link
Collaborator

v1r3n commented Aug 22, 2025

Passing WorkflowDef is going to be OK -- I do not see any performance issues with that, actually it might overall be a little quicker because it completely avoids DB read to get the workflow metadata.

The purpose makes sense, but I think the change itself is a bit too complex. We are internally discussing to see if there is a simpler way. The easiest thing to do here might to be be able to set the parameterized values for task definition.

@hchepey-clari
Copy link
Contributor Author

Any update on this @v1r3n ?

1 similar comment
@hchepey-clari
Copy link
Contributor Author

Any update on this @v1r3n ?

@hchepey-clari
Copy link
Contributor Author

@v1r3n @manan164 - Any update on this?

@v1r3n
Copy link
Collaborator

v1r3n commented Oct 9, 2025

Hi @hchepey-clari the best way to handle this is via dynamic workflows, which conductor already supports.

@hchepey-clari
Copy link
Contributor Author

Ok. Can you help me understand the issues with the changes in these PR that I might have missed out? May be I am missing the bigger picture or something, I am not sure. Can you help me understand and bridge that gap, please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants