Simplified summary process without summarizer nodes - #27953
Draft
Navin Agarwal (agarwal-navin) wants to merge 11 commits into
Draft
Simplified summary process without summarizer nodes#27953Navin Agarwal (agarwal-navin) wants to merge 11 commits into
Navin Agarwal (agarwal-navin) wants to merge 11 commits into
Conversation
Adds ISummaryBuilder/SummaryBuilder and a separate summarize2 flow where the container runtime owns the latest successful summary sequence number and child nodes only track when they last changed. The existing summarize API is unchanged; nodes that have not migrated fall back to it.
…coverage - ISummarizable declares summarize2 once, with docs; IChannel and IFluidDataStoreChannel pick it up via Partial<ISummarizable>. - SharedObject/FluidDataStoreRuntime expose summarize2 as optional properties, so the addition is purely additive and needs no type-test exceptions. - Every DDS now implements summarizeCore2, sharing one serialization routine with summarizeCore via ISummaryContentSink. - summarize2 is gated behind Fluid.ContainerRuntime.EnableSummarizeV2 and skips summarizer node bookkeeping when enabled. - getMetadata takes the summary number instead of incrementing it. - SummaryBuilder computes stats per node, so markUnreferenced is order-independent. - Adds src/summary/summarizeV2.md describing the architecture and rollout.
The summarize2 flow fell back to the old summarize API for nodes that had not implemented the new one, so a single summary could be built by both flows. Part of the tree would then be produced with different semantics (no incremental reuse, different realization and telemetry behavior), which makes comparing the two flows meaningless. Every participant must now implement the new API; if one does not, summarization asserts instead of silently falling back. The addSummaryTreeToBuilder bridge is removed as it existed only to copy old-flow output into the builder. Also gives FluidDataStoreRuntime an overridable summarizeCore2 and implements it in mixinSummaryHandler, which would otherwise have silently dropped its injected summary content under summarize2.
The flows cannot be compared within a client, since only one runs per summary, so they are compared across cohorts of clients instead. That needs every metric tagged with the cohort and computed the same way for both flows. - summarizeFlow is a persistent property on the summarizer logger, so it tags every summarizer event, including failures that never reach summary generation. It is also on IGeneratedSummaryStats. - realizedDataStoreCount, channelCount and reusedChannelCount added to IGeneratedSummaryStats, derived from the generated summary tree or from context state common to both flows rather than from either flow's internals. - A node that cannot participate in the new flow now throws a UsageError carrying its type and id, instead of a bare assert. The feature gate key is shared between the runtime and the summarizer so the flow that runs and the flow that is reported cannot disagree.
github-actions
Bot
added
area: tools
area: runtime
Contributor
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (2478 lines, 64 files), I've queued these reviewers:
How this works
|
IGeneratedSummaryStats gained the summarizeFlow and reuse-count fields after the changeset was written.
Without summarizer nodes, refreshLatestSummaryAck always took the "summary not tracked by this client" path, because summarizerNode.refreshLatestSummary has no record of a summary the v2 flow produced. That path refetches the snapshot, waits closeSummarizerDelayMs (5s) and then disposes the summarizer - after every successful summary. End to end this showed up as summaries taking ~5s instead of ~80ms, blowing the 2s test budget and hanging the summarize-retry tests. The runtime now decides whether the ack is its own by remembering the handle it uploaded. Found by enabling the flow by default and running the local-server e2e suite.
Found by enabling the flow by default and running the local-server e2e suite, which went from 33 failures to 9. Version boundary: a container can hold a data store runtime from a release that predates summarize2. The DataStore layer now advertises a summarize2 layer-compat feature and FluidDataStoreContext uses the old summarize API for a data store whose runtime does not. Channels and shared objects fall back the same way. Such a data store is written in full - its channels cannot be incremental either, since their summarizer nodes are only usable when the summary was started through the summarizer node tree (calling summarize with trackState there asserts 0x5df). Reference state: a data store's unreferenced flag is part of its summary, so a change in used routes has to count as a change even when content did not change. The first observation in a session has no local baseline and uses the summarizer node's hasUsedStateChanged(), whose reference used routes come from the base snapshot; that is what catches a reference state which changed before this client loaded. Also fixes the GC sweep tests, which injected summary failures by monkey-patching containerRuntime.summarize and so never failed under the new flow.
github-actions
Bot
added
the
area: tests
The schema summarizer previously got its incremental reuse from IExperimentalIncrementalSummaryContext.summaryPath, which the summarize2 flow does not provide, so the stored schema was rewritten on every summary. Add an optional canReuseSummary(latestSummarySequenceNumber) to Summarizable and implement it on the schema summarizer by comparing against the sequence number at which the stored schema last changed. summarizeCore2 reuses the whole summarizable subtree via createBuilderForChild(key).nodeDidNotChange(), which also preserves the version metadata blob, so a summary keeps the format it was last written with until the schema actually changes. Also add a missing ISummaryTree import in dataStoreContext.ts that incremental tsc had been masking, and update summarizeV2.md to record that the schema summarizer is ported and to describe the remaining work for the forest summarizer.
Navin Agarwal (agarwal-navin)
changed the title
Navin Agarwal (agarwal-navin)
changed the title
Navin Agarwal (agarwal-navin)
changed the title
Navin Agarwal (agarwal-navin)
changed the title
summarize2 was suffixed only to avoid colliding with the existing summarize
during rollout, which meant it had to be renamed again once summarize was
removed. generateSummary does not collide, so it is a keeper: the rollout ends
by deleting summarize rather than by a second rename of released API.
The verb also matches what the summarize pipeline already calls this step in
its telemetry ("generateSummary" in summaryGenerator).
Renames summarizeCore2 to generateSummaryCore across the DDSes for the same
reason, and the layer compatibility feature to match.
Also removes unused imports in channelContext.ts and sharedObjectKernel.ts that
incremental TypeScript builds were masking - these fail a clean build.
Navin Agarwal (agarwal-navin)
changed the title
The recorded totals were stale and one entry pointed at the wrong test. Measured against the local driver with the gate on, the numbers are 582 passing and 10 failing, not 583 and 9. The summary-handle-resolution category is two tests rather than one, and they are in tree.spec.ts, not summarizeWithOutOfOrderDataStoreRealization.spec.ts - the test listed there is not among the failures. Record what they actually assert and leave the resolution open, since a data store attached via op having no previous summary for its subtree looks more like a missing never-summarized sequence number on the attach path than something to relax the tests about.
Contributor
Bundle size comparisonBase commit: Pending — |
Contributor
|
🔗 Found some broken links! 💔 Run a link check locally to find them. See Checking for Broken Links for more information. linkcheck output |
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
1 participant
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.
Description
Summary state today is spread across a tree of
SummarizerNodes. Every node keeps its own copy of "what did Ilast summarize, and when", advanced through a multi-stage protocol (
startSummary→summarize→validateSummary→completeSummary→refreshLatestSummary/clearSummary). That duplication is why nodescan disagree with the runtime, why a failed or nacked summary has to be rolled back across the tree, and why
nodes get realized just to take part in the protocol.
This adds a second flow that keeps one piece of state on the container runtime -
latestSummarySequenceNumber- while each node tracks only when its own content last changed. Summarizingbecomes a single pass with one comparison per node and no per-node state machine, and a failed summary needs no
rollback because the decision input only advances on ack.
Nodes write into an
ISummaryBuilderinstead of returning a tree for the parent to merge. The builder deriveshandle paths from the tree structure, so nodes no longer need to be told their own path - removing the
summaryPathleakage inIExperimentalIncrementalSummaryContext.The new flow is off by default behind
Fluid.ContainerRuntime.EnableSummarizeV2. The existingsummarizeAPI is untouched, and a summary is produced entirely by one flow or the other - never a mix, since partial trees
built with different semantics would make the two impossible to compare.
Main pieces:
ISummaryBuilder/SummaryBuilder, andISummarizabledeclaringgenerateSummaryonce (IChannelandIFluidDataStoreChannelpick it up viaPartial<ISummarizable>, so it stays optional for externalimplementers).
ContainerRuntime.generateSummary, plus the gate insubmitSummarywhich also skips the summarizer-nodebookkeeping that no longer applies.
generateSummaryCoreimplemented across all 15 DDS packages, sharing one serialization routine withsummarizeCoreviaISummaryContentSinkso the two cannot drift.summarizeatthe version boundary instead of failing.
summarizeFlowon every summarizer event, plusrealizedDataStoreCount,channelCountandreusedChannelCountonIGeneratedSummaryStats. SeeDashboard signals.
On naming:
generateSummaryis named for what it does rather than being a suffixed variant ofsummarize,so it does not need renaming when it replaces
summarizeat the end of the rollout - the old method is simplyremoved. The
generateverb matches the name the summarize pipeline already uses for this step in its telemetry.Not ported yet: SharedTree's forest incremental summarization still depends on
summaryPath, so the forestis written in full under
generateSummary. SharedTree's schema summarizer is ported, via an optionalSummarizable.canReuseSummarythat lets an unchanged summarizable reuse its whole subtree. Both are called outin the doc.
Reviewer Guidance
The review process is outlined on this wiki page.
Specific things I'd like opinions on:
generateSummaryis an optional property, not a method, onSharedObjectandFluidDataStoreRuntime. Thatkeeps the addition purely additive (no type-test exceptions needed), but a property can't be overridden via
super, so each delegates to an overridablegenerateSummaryCore. Is that trade the right one, or would yourather take the class-shape break and have a plain method?
that predates the API advertises no support and is summarized with
summarize, so the two flows can coexistacross a version boundary within one container. Worth a look at whether that check is in the right place.
summarizer-node internals that this flow removes. I'd like agreement on which should be deleted versus adapted
before this leaves draft.
Validation
Clean
tsc --noEmit, lint, format and API reports across all touched packages.The 10 remaining failures fall into four groups, all inventoried in
the doc:
version-boundary cases where a legacy data store or a DDS relying on
IExperimentalIncrementalSummaryContextiswritten in full; two tests asserting
SummarizerNoderefresh telemetry this flow never emits; two asserting aNodeDidNotSummarizepath that no longer exists; and two summary-handle-resolution tests. None is unexplained.