Completed batch waitpoints when we completed the BatchTaskRun #1945
Conversation
|
WalkthroughThe changes refactor how batch completion and parent run unblocking are handled in the run engine system. The explicit method for unblocking a parent run after batch creation is removed from the Changes
Sequence Diagram(s)sequenceDiagram
participant RunEngine
participant BatchSystem
participant WaitpointSystem
RunEngine->>BatchSystem: createBatch(...)
BatchSystem->>BatchSystem: #tryCompleteBatch()
BatchSystem->>BatchSystem: Mark batch as COMPLETED
BatchSystem->>WaitpointSystem: completeWaitpoint(waitpointId, output) (if waitpoint exists)
Suggested reviewers
Poem
✨ 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 (2)
internal-packages/run-engine/src/engine/systems/batchSystem.ts (2)
20-29: Reduced job delay from 2000ms to 200msThe delay for the
tryCompleteBatchjob has been significantly reduced from what appears to be 2 seconds to 200ms. This should help address the high contention issues mentioned in the PR objectives.Consider adding a comment explaining why this specific delay was chosen and how it helps with the contention issues.
82-104: Implementation of waitpoint completion logicThe implementation correctly queries for associated waitpoints and completes them when a batch is completed. This aligns with the PR objective of moving waitpoint completion to happen when the BatchTaskRun is completed.
The debug message on line 92 still refers to "RunEngine.unblockRunForBatch()" which should be updated to reflect that this is now in the BatchSystem class, perhaps to "BatchSystem.#tryCompleteBatch()".
- this.$.logger.debug( - "RunEngine.unblockRunForBatch(): Waitpoint not found. This is ok, because only batchTriggerAndWait has waitpoints", - { - batchId, - } - ); + this.$.logger.debug( + "BatchSystem.#tryCompleteBatch(): Waitpoint not found. This is ok, because only batchTriggerAndWait has waitpoints", + { + batchId, + } + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal-packages/run-engine/src/engine/systems/batchSystem.ts(3 hunks)internal-packages/run-engine/src/engine/tests/batchTriggerAndWait.test.ts(1 hunks)internal-packages/run-engine/src/engine/tests/checkpoints.test.ts(0 hunks)
💤 Files with no reviewable changes (1)
- internal-packages/run-engine/src/engine/tests/checkpoints.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
internal-packages/run-engine/src/engine/tests/batchTriggerAndWait.test.ts (1)
299-299: Increased timeout to accommodate asynchronous batch completionThe timeout has been increased from 500ms to 1000ms, which is appropriate given the changes to the batch completion mechanism. With the batch waitpoint completion now handled asynchronously in the BatchSystem with a 200ms delay, this additional wait time ensures that all operations complete before assertions are checked.
internal-packages/run-engine/src/engine/systems/batchSystem.ts (3)
4-4: Added dependency on WaitpointSystemThe import of WaitpointSystem is appropriate as part of moving the waitpoint completion responsibility into the BatchSystem.
6-9: Updated BatchSystemOptions to include WaitpointSystemGood addition of the waitpointSystem dependency to the options interface.
11-18: Proper initialization of WaitpointSystem dependencyThe addition of the waitpointSystem property and its initialization in the constructor are well-implemented.
We were previously completing these waitpoints when we thought all runs had been triggered. There's high contention in the code here.
Some of these waitpoints were not getting completed, even though they should have been. For all of the ones I saw the
BatchTaskRunwas completed, so I've moved the waitpoint completion code to there instead.Summary by CodeRabbit
New Features
Refactor