Offline: Add batchId to batch metadata on resubmit #21767
Conversation
PendingStateManager is mostly done
github-actions
bot
added
area: runtime
markfields
requested review from
anthony-murphy,
kian-thompson,
dannimad,
vladsud,
a team,
pragya91,
agarwal-navin and
jatgarg
and removed request for
a team
| // Only include Batch ID if "Offline Load" feature is enabled | ||
| // It's only needed to identify batches across container forks arising from misuse of offline load. | ||
| const includeBatchId = | ||
| this.mc.config.getBoolean("Fluid.Container.enableOfflineLoad") ?? false; |
There was a problem hiding this comment.
It's interesting that we're reading this flag from two different places. I don't know if that's a good or bad thing. And it always makes me worry about loader-runtime back compat with older drivers and how they might behave.
There was a problem hiding this comment.
Two different places in this PR? I thought it was just here.
This flag is read in multiple places and is the single gate for all Offline related scenarios. Good question about loader/runtime... Yeah... I see what you mean, the other hit is in Loader layer. I'll follow up, but leaving here for now since none of this has shipped yet.
| export function asBatchMetadata(metadata: unknown): IBatchMetadata | undefined { | ||
| return isBatchMetadata(metadata) ? metadata : undefined; | ||
| return metadata as IBatchMetadata | undefined; |
There was a problem hiding this comment.
For some reason, I favored the older solution as it seemed a little safer, but I don't know if this is important.
There was a problem hiding this comment.
Yeah I had just added that type guard, but on second though didn't think the runtime checks were that interesting. The only risk is if two different parts of code used the same key batch or batchId and stored data with a different type and different semantics. That's not going to happen though... So let's not waste time with the runtime checks.
Since all IBatchMetadata props are optional, and this thing indicates it could be undefined, callers have to handle those cases.
Maybe it's better to return Partial<IBatchMetadata> | undefined to emphasize the uncertainty (basically just want callers to be able to get intellisense on valid property names + expected types)
⯅ @fluid-example/bundle-size-tests: +2.55 KB
Baseline commit: 0450953 |
Description
Batch ID is the key piece of information needed to detect/prevent duplication of changes due to a forked container.
We compute Batch ID as a tuple of original client ID and original Client Sequence number (of the batch start). We only stamp the batchId on batch metadata (of first message in the batch) on resubmit - if a batch is missing batchId we will be able to compute it from the client ID and Client Sequence Number.
The next PR will be more interesting - comparing batch IDs in PendingStateManager as incoming messages are processed. This just sets it up by adding batch ID to batch metadata on resubmit.
Breaking Changes
This PR breaks the format of the stashed pending local state. Back-compat support for reading old-format stashes is included. New format is backwards compatible (additive). This is slightly superfluous anyway since there is no production usage of the stashed schema.
Reviewer Guidance
For the end-to-end flow, see prototype PR #21266
Here are some tests I considered in my test plan but I am not planning to write at this point (i.e. scenarios without direct test coverage). Many of these will be covered in the next PR where we have end-to-end coverage of the forked container scenarios.
batchIdContext !== undefined