Skip to content

Add adaptive concurrency parameter for queue-worker#1245

Open
welteki wants to merge 1 commit intoopenfaas:masterfrom
welteki:adaptive-concurrency
Open

Add adaptive concurrency parameter for queue-worker#1245
welteki wants to merge 1 commit intoopenfaas:masterfrom
welteki:adaptive-concurrency

Conversation

@welteki
Copy link
Member

@welteki welteki commented Sep 18, 2025

Description

Allow users to disable the new adaptive concurrency feature for the queue-worker.

New parameter in OpenFaaS chart:

Parameter Description Default
jetstreamQueueWorker.adaptiveConcurrency Enable adaptive concurrency limiting for functions. This setting only takes effect when jetstreamQueueWorker.mode is set to function. true

New parameter in queue-worker chart:

Parameter Description Default
adaptiveConcurrency Enable adaptive concurrency limiting for functions. This setting only takes effect when mode is set to function. true

Why is this needed?

  • I have raised an issue to propose this change (required)

Support configuration for new adaptive concurrency feature.

Who is this for?

What company is this for? Are you listed in the ADOPTERS.md file?

How Has This Been Tested?

Used the OpenFaaS chart with the new parameter to deploy during testing. Verified the parameter was set.

Verified the queue-worker chart by running: helm template ./chart/queue-worker

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@welteki welteki force-pushed the adaptive-concurrency branch from e755985 to 383859a Compare October 7, 2025 08:53
@welteki welteki force-pushed the adaptive-concurrency branch from 383859a to 396950c Compare January 21, 2026 14:13
@welteki welteki force-pushed the adaptive-concurrency branch from 396950c to 5fe3571 Compare February 27, 2026 13:27
@reviewfn

This comment has been minimized.

@@ -99,6 +99,8 @@ spec:
- name: ack_wait
value: "{{ .Values.queueWorker.ackWait }}"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to if guard these blocks so there is no output when not defined?

{{- will avoid white space from conditional blocks.

Allow users to disable the new adaptive concurrency feature for the
queue-worker.

Signed-off-by: Han Verstraete (OpenFaaS Ltd) <han@openfaas.com>
@welteki welteki force-pushed the adaptive-concurrency branch from 5fe3571 to 70687f5 Compare February 27, 2026 13:36
@reviewfn
Copy link

reviewfn bot commented Feb 27, 2026

AI Pull Request Overview

Summary

  • Adds adaptiveConcurrency parameter to both OpenFaaS and queue-worker Helm charts to allow disabling the new adaptive concurrency feature
  • Parameter defaults to true and is only effective when queue mode is set to function
  • Updates chart values, templates, and documentation
  • Passes the setting as limiter_enabled environment variable to the queue-worker deployment
  • Tested manually via Helm deployment and template rendering
  • No automated tests added for the configuration changes

Approval rating (1-10)

7/10 - Solid configuration addition for feature control, but lacks automated testing and potential behavior change implications.

Summary per file

Summary per file
File path Summary
chart/openfaas/README.md Adds documentation for new adaptiveConcurrency parameter
chart/openfaas/templates/queueworker-dep.yaml Adds limiter_enabled env var based on adaptiveConcurrency value
chart/openfaas/values.yaml Adds adaptiveConcurrency default value with comments
chart/queue-worker/README.md Adds documentation for adaptiveConcurrency parameter
chart/queue-worker/templates/deployment.yaml Adds limiter_enabled env var based on adaptiveConcurrency value
chart/queue-worker/values.yaml Adds adaptiveConcurrency default value with comments

Do not include node_modules or vendor directories.

Overall Assessment

The PR introduces a straightforward configuration option to control the adaptive concurrency feature, which is appropriate for allowing users to opt-out. However, the lack of automated tests increases regression risk, and the default enabling of the feature may introduce unexpected behavior changes for enterprise users. The implementation is consistent across both charts, but verification of the environment variable name and its handling in the queue-worker code is recommended.

Detailed Review

Detailed Review

chart/openfaas/README.md

  • Correctly documents the new parameter with description and default value
  • Placement in the table is appropriate, following existing parameter order

chart/openfaas/templates/queueworker-dep.yaml

  • Adds the limiter_enabled environment variable correctly mapped to .Values.jetstreamQueueWorker.adaptiveConcurrency
  • Positioning in the env list is consistent with other queue worker settings
  • No issues with template syntax

chart/openfaas/values.yaml

  • Adds the parameter under the correct jetstreamQueueWorker section
  • Comments provide clear explanation and condition for when it takes effect
  • Default value of true aligns with enabling the new feature by default

chart/queue-worker/README.md

  • Accurately documents the parameter with matching description and default
  • Minor formatting change: trailing space removed from upstreamTimeout: 15m - this appears unintentional and should be reverted if not desired
  • Parameter integration in the example and table is proper

chart/queue-worker/templates/deployment.yaml

  • Correctly adds limiter_enabled env var mapped to .Values.adaptiveConcurrency
  • Consistent with the openfaas chart implementation
  • No template issues

chart/queue-worker/values.yaml

  • Adds the parameter at the appropriate level in the values hierarchy
  • Comments match those in the openfaas chart for consistency
  • Default value is consistent

General Issues

  • Testing: No automated tests were added despite the PR checklist indicating tests are required for changes. Given this is configuration-only, unit tests for chart validation or integration tests could mitigate risks of misconfiguration or regressions. Manual testing is insufficient for long-term maintainability.
  • Environment Variable Verification: The PR assumes limiter_enabled is the correct env var name used by the queue-worker code. Without access to the codebase, this cannot be verified - recommend cross-referencing with the queue-worker source to ensure accuracy.
  • Migration Impact: Defaulting to true enables the feature automatically, which could change performance characteristics for existing deployments using function mode. While this adds functionality, it may cause unexpected resource usage or behavior changes. Consider documenting migration notes or making it opt-in for safer adoption.
  • Consistency: The parameter naming and behavior are consistent between the two charts, which is good. However, the scoping (nested vs top-level) reflects the chart structure appropriately.
  • Security/Performance: No obvious security concerns. The feature is intended to improve concurrency handling, but enabling by default assumes the adaptive logic is robust and doesn't introduce instability.
  • Prioritization:
    • High: Verify limiter_enabled env var correctness in queue-worker code to prevent configuration failures
    • Medium: Add automated chart tests or CI validation for the new parameter
    • Low: Revert unintended whitespace change in README.md

AI agent details.

Agent processing time: 33.097s
Environment preparation time: 8.025s
Total time from webhook: 54.326s

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