Add settle window to DataLoader batch dispatch#10129
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a configurable settle window to BatchDispatcher so batches become dispatch-eligible only after a minimum quiet period, improving aggregation under concurrent load while preserving age-based forced dispatch semantics.
Changes:
- Added
BatchSettleTimeUsplus “was set” tracking toBatchDispatcherOptionssodefault(BatchDispatcherOptions)yields documented defaults. - Updated
BatchDispatcherdispatch eligibility and coordinator loop to incorporate settle time and replace the prior idle-cycle heuristic with a time-based no-dispatch streak/backoff strategy. - Added/updated tests for settle window behavior and option default handling, plus an additional experiment-style test file.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/HotChocolate/Core/src/Types/Fetching/BatchDispatcher.cs | Adds settle-window gating to dispatch eligibility and revises coordinator backoff/evaluation signaling. |
| src/HotChocolate/Core/src/Types/Fetching/BatchDispatcherOptions.cs | Adds BatchSettleTimeUs and explicit default handling for default(...) scenarios. |
| src/HotChocolate/Core/src/Types/Fetching/BatchDispatcher.IBatchScheduler.cs | Removes now-unused enqueue timestamp tracking. |
| src/HotChocolate/Core/src/Types/Fetching/BatchDispatcher.ExecutorSession.cs | Removes now-unused subscribe timestamp tracking. |
| src/HotChocolate/Core/test/Fetching.Tests/BatchDispatcherTests.cs | Adds tests for settle window inclusion of late items and default/zero option semantics. |
| src/HotChocolate/Core/test/Execution.Tests/Integration/DataLoader/DataLoaderBatchAggregationExperimentTests.cs | Adds measurement-only experiment tests for batch aggregation behavior under load. |
Comments suppressed due to low confidence (2)
src/HotChocolate/Core/src/Types/Fetching/BatchDispatcherOptions.cs:19
- The XML doc says "class" but
BatchDispatcherOptionsis astruct. Updating this avoids misleading generated docs.
/// <summary>
/// Initializes a new instance of the <see cref="BatchDispatcherOptions"/> class.
/// </summary>
src/HotChocolate/Core/src/Types/Fetching/BatchDispatcher.cs:165
- This comment implies we only return when in-flight dispatches are completed, but the code returns whenever
openBatches == 0(relying on dispatch completion continuations to re-signal the coordinator). Please update the comment to match the actual control flow.
// If we have no open batches to evaluate and all in-flight dispatches
// are completed, we can stop and wait for another signal.
// If there are in-flight dispatches still running we also stop and
// wait for their completion signal.
if (openBatches == 0)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Patch coverage100.0% of changed lines covered (46/46)
Project coverage: 53.8% (231332/430063 lines) |
The file was scaffolding for GitHub discussion #9793 and, per its own header, was not meant to be committed. The tests assert nothing and always pass, so they add CI runtime without providing regression coverage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a configurable settle window to the
BatchDispatcherso a batch only becomes eligible for dispatch once it has gone a minimum amount of time without receiving new items. This widens the aggregation window under concurrent load, so fewer, larger batches reach the DataLoader.Closes #9793