-
Notifications
You must be signed in to change notification settings - Fork 576
feat(container-runtime): enable batchId tracking by default with kill-switch and perf telemetry #27216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(container-runtime): enable batchId tracking by default with kill-switch and perf telemetry #27216
Changes from 6 commits
9d2af54
446452d
1050c3d
f674df1
a08023a
b29d0ff
031056a
90edb00
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1890,20 +1890,34 @@ export class ContainerRuntime | |||||||||||||||||||
| this.mc.config.getNumber("Fluid.ContainerRuntime.StagingModeAutoFlushThreshold") ?? | ||||||||||||||||||||
| runtimeOptions.stagingModeAutoFlushThreshold ?? | ||||||||||||||||||||
| defaultStagingModeAutoFlushThreshold; | ||||||||||||||||||||
| this.batchIdTrackingEnabled = | ||||||||||||||||||||
| this.mc.config.getBoolean("Fluid.Container.enableOfflineFull") ?? | ||||||||||||||||||||
| this.mc.config.getBoolean("Fluid.ContainerRuntime.enableBatchIdTracking") ?? | ||||||||||||||||||||
| false; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if (this.batchIdTrackingEnabled && this._flushMode !== FlushMode.TurnBased) { | ||||||||||||||||||||
| // BatchId tracking powers DuplicateBatchDetector (catching forked-container duplicates) | ||||||||||||||||||||
| // and is also a prerequisite for the Offline Load feature. It is enabled by default in | ||||||||||||||||||||
| // TurnBased mode; the kill-switch below allows disabling it without a code change if a | ||||||||||||||||||||
| // regression is observed. Offline Load still requires TurnBased, so consumers that | ||||||||||||||||||||
| // explicitly opt into it with FlushMode.Immediate continue to get a UsageError. | ||||||||||||||||||||
| const offlineLoadRequested = | ||||||||||||||||||||
| this.mc.config.getBoolean("Fluid.Container.enableOfflineFull") === true; | ||||||||||||||||||||
| const disableBatchIdTracking = | ||||||||||||||||||||
| this.mc.config.getBoolean("Fluid.ContainerRuntime.DisableBatchIdTracking") === true; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if (offlineLoadRequested && this._flushMode !== FlushMode.TurnBased) { | ||||||||||||||||||||
| const error = new UsageError("Offline mode is only supported in turn-based mode"); | ||||||||||||||||||||
| this.closeFn(error); | ||||||||||||||||||||
| throw error; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
||||||||||||||||||||
| if (offlineLoadRequested && disableBatchIdTracking) { | |
| const error = new UsageError( | |
| "Offline mode requires batchId tracking and cannot be used when Fluid.ContainerRuntime.DisableBatchIdTracking is enabled.", | |
| ); | |
| this.closeFn(error); | |
| throw error; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
offline load flag will also by removed in a follow up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deep Review: Following up on this thread now that batchId tracking is default-on. The asymmetry the original comment flagged is still live in the current diff: the offlineLoadRequested UsageError validates only flush mode, not the kill-switch, and the constructor comment immediately above still describes batchId tracking as the prerequisite for Offline Load. Pre-PR, enableOfflineFull=true unconditionally enabled tracking; post-PR, setting enableOfflineFull=true together with Fluid.ContainerRuntime.DisableBatchIdTracking=true in TurnBased silently strips the prerequisite — Offline Load proceeds with fork detection disabled, no error, no telemetry. The PR description says Offline Load is "back-compat preserved" but only the FlushMode portion currently is.
@dannimad you mentioned enableOfflineFull will be removed in a follow-up — that resolves this cleanly if it's imminent. A couple of cheap options either way:
- (a) Link the follow-up issue here and add a one-line comment in the constructor noting the asymmetry is intentionally transient, or
- (b) Add the symmetric guard now — extend the existing
UsageErrorto fire whenofflineLoadRequested && disableBatchIdTracking, or emit aBatchIdTrackingDisabledForOfflineLoadwarning telemetry so the misconfig is at least observable while the follow-up lands.
Either closes the loop on this thread.
Uh oh!
There was an error while loading. Please reload this page.