re2: add ability to selectively disable release concurrency queue consumers via env var #1881
Conversation
…eue consumers via env var Also added some additional logging
|
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request adds a new configuration option to control consumer behavior in the run engine's release concurrency system. An environment variable ( Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Environment
participant RE as RunEngine
participant RCS as ReleaseConcurrencySystem
participant RTBQ as ReleaseConcurrencyTokenBucketQueue
participant Cons as Consumers
Env->>RE: Provide RUN_ENGINE_RELEASE_CONCURRENCY_DISABLE_CONSUMERS
RE->>RCS: Configure releaseConcurrency (disableConsumers flag)
RCS->>RTBQ: Pass options with disableConsumers
alt disableConsumers is true
RTBQ-->>Cons: Skip starting consumers
else
RTBQ->>Cons: Start consumers
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/webapp/app/env.server.ts (1)
570-570: Added new environment variable for controlling release concurrency consumers.This change adds the
RUN_ENGINE_RELEASE_CONCURRENCY_DISABLE_CONSUMERSenvironment variable with a default value of "0" (enabled). This allows operators to selectively disable release concurrency queue consumers by setting it to "1".Consider adding a comment explaining the purpose of this environment variable and what disabling consumers would mean for the system operation, similar to other environment variables in this file.
+ /** If set to "1", disables the release concurrency queue consumers, allowing manual control */ RUN_ENGINE_RELEASE_CONCURRENCY_DISABLE_CONSUMERS: z.string().default("0"),internal-packages/run-engine/src/engine/releaseConcurrencyTokenBucketQueue.ts (2)
31-31: Consider adding documentation for the disableConsumers option.While the implementation is correct, adding a JSDoc comment to describe the purpose and behavior of the
disableConsumersoption would improve discoverability and usability of this feature.+ /** + * When set to true, prevents the queue from starting consumers. + * This can be useful in scenarios where you want to control concurrency + * without processing the queue automatically. + */ disableConsumers?: boolean;Also applies to: 78-81
78-81: Consider more explicit boolean check for disableConsumers.For improved clarity and robustness, consider using a strict equality check.
- if (!options.disableConsumers) { + if (options.disableConsumers !== true) { this.#startConsumers(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/v3/runEngine.server.ts(1 hunks)internal-packages/run-engine/src/engine/index.ts(1 hunks)internal-packages/run-engine/src/engine/releaseConcurrencyTokenBucketQueue.ts(11 hunks)internal-packages/run-engine/src/engine/types.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
internal-packages/run-engine/src/engine/index.ts (1)
apps/webapp/app/runEngine/services/batchTrigger.server.ts (1)
options(645-650)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
🔇 Additional comments (11)
internal-packages/run-engine/src/engine/types.ts (1)
54-54: Addition of a new configuration option for controlling concurrency queue consumers.This change adds a new optional boolean property
disableConsumersto thereleaseConcurrencyconfiguration object, allowing selective disabling of release concurrency queue consumers.apps/webapp/app/v3/runEngine.server.ts (1)
80-80: Implementation of env var mapping for disabling concurrency queue consumers.This change maps the
RUN_ENGINE_RELEASE_CONCURRENCY_DISABLE_CONSUMERSenvironment variable to the newdisableConsumersproperty in the run engine configuration. Consumers will be disabled when the environment variable equals "1".internal-packages/run-engine/src/engine/index.ts (1)
204-204:✅ Verification successful
Propagating the disableConsumers configuration to the ReleaseConcurrencySystem.
This change passes the
disableConsumerssetting from the engine options to thequeueOptionsobject used by theReleaseConcurrencySystem. The implementation is clean and maintains the existing conditional structure.Let's make sure the
disableConsumersconfiguration is properly used in the ReleaseConcurrencySystem. Run this script to check how the property is used:
🏁 Script executed:
#!/bin/bash # Search for how disableConsumers is used in the code rg -A 5 $'disableConsumers' --type ts internal-packages/run-engine/srcLength of output: 2284
Verified:
disableConsumersPropagation Works as IntendedAfter reviewing the code and search results, I can confirm that:
- The
disableConsumersproperty is properly defined ininternal-packages/run-engine/src/engine/types.ts.- It is correctly passed from the engine options in
internal-packages/run-engine/src/engine/index.tsvia the line:disableConsumers: options.releaseConcurrency?.disableConsumers,- The property is effectively utilized in the
ReleaseConcurrencySystemimplementation (specifically ininternal-packages/run-engine/src/engine/releaseConcurrencyTokenBucketQueue.ts), where the consumer startup is conditionally executed based onif (!options.disableConsumers).The implementation is clear, and the propagation maintains the intended conditional structure. No further changes are required.
internal-packages/run-engine/src/engine/releaseConcurrencyTokenBucketQueue.ts (8)
31-31: Good addition of the disableConsumers configuration option.The new optional property allows for selectively disabling queue consumers, which aligns well with the PR objectives.
78-81: Looks good - conditional consumer startup successfully implemented.This conditional logic ensures consumers are only started when not explicitly disabled, maintaining backward compatibility while adding the new feature.
100-105: Enhanced debug logging improves traceability.Adding detailed debug logging here is valuable for diagnostics when no tokens are available. The structured logging with relevant context (releaseQueueDescriptor, releaserId, maxTokens) will help troubleshoot release concurrency issues.
122-128: Useful debug logging for token consumption.This additional logging provides visibility into the token consumption process, which is important for tracking concurrency behavior.
152-152: Fixed code duplication and added debug logging.Good reorganization - the redundant definition of
releaseQueuewas removed by moving it up before the conditional check, and informative debug logging was added.Also applies to: 155-161
176-181: Added helpful debug logging to consumeToken method.Consistent debug logging pattern provides better visibility into token consumption.
192-195: Improved traceability with before/after logging in returnToken.Adding debug logs both before and after the token return operation provides valuable data about the operation's lifecycle, making it easier to diagnose issues.
Also applies to: 206-210
223-226: Added comprehensive debug logging for refillTokens edge cases and success.The logging additions in refillTokens offer improved visibility into:
- Invalid cases (negative or zero tokens)
- Successful token refill operations
This enhances the observability of the token bucket system and will help with troubleshooting concurrency issues.
Also applies to: 232-235, 249-254
Also added some additional logging
Summary by CodeRabbit
TaskRunProcess.