From 5a7338ef01ab04623d21e3cab9eb570f6c2df2dc Mon Sep 17 00:00:00 2001 From: Navin Agarwal Date: Wed, 30 Aug 2023 20:38:08 +0000 Subject: [PATCH 1/5] Removed PendingSummaryNotFound telemetry log --- .../src/summary/summarizerNode/summarizerNode.ts | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNode.ts b/packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNode.ts index a9877f61346a..503779df5202 100644 --- a/packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNode.ts +++ b/packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNode.ts @@ -423,17 +423,6 @@ export class SummarizerNode implements IRootSummarizerNode { summaryRefSeq, }; } - - const props = { - summaryRefSeq, - pendingSize: this.pendingSummaries.size ?? undefined, - }; - this.logger.sendTelemetryEvent({ - eventName: "PendingSummaryNotFound", - proposalHandle, - referenceSequenceNumber: this.referenceSequenceNumber, - details: JSON.stringify(props), - }); } // If the summary for which refresh is called is older than the latest tracked summary, ignore it. From 682762ed1890e0cd5b16ddf4df142eaddb3b72c0 Mon Sep 17 00:00:00 2001 From: Navin Agarwal Date: Thu, 31 Aug 2023 17:32:14 +0000 Subject: [PATCH 2/5] Update refresh params and result --- .../container-runtime/src/containerRuntime.ts | 10 +-- .../src/gc/garbageCollection.ts | 5 +- .../container-runtime/src/gc/gcDefinitions.ts | 8 +-- .../src/gc/gcSummaryStateTracker.ts | 24 +++---- .../container-runtime/src/summary/index.ts | 1 - .../src/summary/summarizerNode/index.ts | 1 - .../summary/summarizerNode/summarizerNode.ts | 62 ++++++------------- .../summarizerNode/summarizerNodeUtils.ts | 29 +-------- .../src/test/gc/garbageCollection.spec.ts | 15 +---- .../src/test/gc/gcSummaryStateTracker.spec.ts | 37 ++--------- .../src/test/summarizerNode.spec.ts | 19 +++--- .../src/test/summarizerNodeWithGc.spec.ts | 26 ++++---- 12 files changed, 69 insertions(+), 168 deletions(-) diff --git a/packages/runtime/container-runtime/src/containerRuntime.ts b/packages/runtime/container-runtime/src/containerRuntime.ts index a0a1e04aef92..573d8dfff58b 100644 --- a/packages/runtime/container-runtime/src/containerRuntime.ts +++ b/packages/runtime/container-runtime/src/containerRuntime.ts @@ -3651,19 +3651,21 @@ export class ContainerRuntime /** Implementation of ISummarizerInternalsProvider.refreshLatestSummaryAck */ public async refreshLatestSummaryAck(options: IRefreshSummaryAckOptions) { const { proposalHandle, ackHandle, summaryRefSeq, summaryLogger } = options; + // proposalHandle is always passed from RunningSummarizer. + assert(proposalHandle !== undefined, "proposalHandle should be available"); const readAndParseBlob = async (id: string) => readAndParse(this.storage, id); - const result = await this.summarizerNode.refreshLatestSummary( + const isSummaryTracked = await this.summarizerNode.refreshLatestSummary( proposalHandle, summaryRefSeq, ); /** - * When refreshing a summary ack, this check indicates a new ack of a summary that was newer than the + * When refreshing a summary ack, this check indicates a new ack of a summary that is newer than the * current summary that is tracked, but this summarizer runtime did not produce/track that summary. Thus * it needs to refresh its state. Today refresh is done by fetching the latest snapshot to update the cache * and then close as the current main client is likely to be re-elected as the parent summarizer again. */ - if (result.latestSummaryUpdated && !result.wasSummaryTracked) { + if (!isSummaryTracked && summaryRefSeq > this.summarizerNode.referenceSequenceNumber) { const fetchResult = await this.fetchSnapshotFromStorage( summaryLogger, { @@ -3705,7 +3707,7 @@ export class ContainerRuntime } // Notify the garbage collector so it can update its latest summary state. - await this.garbageCollector.refreshLatestSummary(result); + await this.garbageCollector.refreshLatestSummary(isSummaryTracked); } /** diff --git a/packages/runtime/container-runtime/src/gc/garbageCollection.ts b/packages/runtime/container-runtime/src/gc/garbageCollection.ts index 4a8f279f6964..ec38caa4f427 100644 --- a/packages/runtime/container-runtime/src/gc/garbageCollection.ts +++ b/packages/runtime/container-runtime/src/gc/garbageCollection.ts @@ -29,7 +29,6 @@ import { RuntimeHeaders, } from "../containerRuntime"; import { ClientSessionExpiredError } from "../error"; -import { RefreshSummaryResult } from "../summary"; import { generateGCConfigs } from "./gcConfigs"; import { GCNodeType, @@ -846,8 +845,8 @@ export class GarbageCollector implements IGarbageCollector { * Called to refresh the latest summary state. This happens when either a pending summary is acked or a snapshot * is downloaded and should be used to update the state. */ - public async refreshLatestSummary(result: RefreshSummaryResult): Promise { - return this.summaryStateTracker.refreshLatestSummary(result); + public async refreshLatestSummary(isSummaryTracked: boolean): Promise { + return this.summaryStateTracker.refreshLatestSummary(isSummaryTracked); } /** diff --git a/packages/runtime/container-runtime/src/gc/gcDefinitions.ts b/packages/runtime/container-runtime/src/gc/gcDefinitions.ts index 6f66a94af10d..4ededa61482d 100644 --- a/packages/runtime/container-runtime/src/gc/gcDefinitions.ts +++ b/packages/runtime/container-runtime/src/gc/gcDefinitions.ts @@ -14,11 +14,7 @@ import { } from "@fluidframework/runtime-definitions"; import { ReadAndParseBlob } from "@fluidframework/runtime-utils"; import { ITelemetryLoggerExt } from "@fluidframework/telemetry-utils"; -import { - IContainerRuntimeMetadata, - ICreateContainerMetadata, - RefreshSummaryResult, -} from "../summary"; +import { IContainerRuntimeMetadata, ICreateContainerMetadata } from "../summary"; export type GCVersion = number; @@ -228,7 +224,7 @@ export interface IGarbageCollector { /** Returns the GC details generated from the base snapshot. */ getBaseGCDetails(): Promise; /** Called when the latest summary of the system has been refreshed. */ - refreshLatestSummary(result: RefreshSummaryResult): Promise; + refreshLatestSummary(isSummaryTracked: boolean): Promise; /** Called when a node is updated. Used to detect and log when an inactive node is changed or loaded. */ nodeUpdated( nodePath: string, diff --git a/packages/runtime/container-runtime/src/gc/gcSummaryStateTracker.ts b/packages/runtime/container-runtime/src/gc/gcSummaryStateTracker.ts index 8efa018a471c..8656c834496a 100644 --- a/packages/runtime/container-runtime/src/gc/gcSummaryStateTracker.ts +++ b/packages/runtime/container-runtime/src/gc/gcSummaryStateTracker.ts @@ -13,7 +13,6 @@ import { ISummaryTreeWithStats, } from "@fluidframework/runtime-definitions"; import { mergeStats, SummaryTreeBuilder } from "@fluidframework/runtime-utils"; -import { RefreshSummaryResult } from "../summary"; import { GCVersion, IGCStats } from "./gcDefinitions"; import { generateSortedGCState } from "./gcHelpers"; import { IGarbageCollectionSnapshotData, IGarbageCollectionState } from "./gcSummaryDefinitions"; @@ -267,28 +266,23 @@ export class GCSummaryStateTracker { * Called to refresh the latest summary state. This happens when either a pending summary is acked or a snapshot * is downloaded and should be used to update the state. */ - public async refreshLatestSummary(result: RefreshSummaryResult): Promise { - // If the latest summary was updated and the summary was tracked, this client is the one that generated this - // summary. So, update wasGCRunInLatestSummary. + public async refreshLatestSummary(isSummaryTracked: boolean): Promise { + // If the summary is tracked, this client is the one that generated it. So, update wasGCRunInLatestSummary. // Note that this has to be updated if GC did not run too. Otherwise, `gcStateNeedsReset` will always return // true in scenarios where GC is disabled but enabled in the snapshot we loaded from. - if (result.latestSummaryUpdated && result.wasSummaryTracked) { + if (isSummaryTracked) { this.wasGCRunInLatestSummary = this.configs.shouldRunGC; } - if (!result.latestSummaryUpdated || !this.configs.shouldRunGC) { + if (!isSummaryTracked || !this.configs.shouldRunGC) { return; } - // If the summary was tracked by this client, it was the one that generated the summary in the first place. - // Update latest state from pending. - if (result.wasSummaryTracked) { - this.latestSummaryGCVersion = this.configs.gcVersionInEffect; - this.latestSummaryData = this.pendingSummaryData; - this.pendingSummaryData = undefined; - this.updatedDSCountSinceLastSummary = 0; - return; - } + this.latestSummaryGCVersion = this.configs.gcVersionInEffect; + this.latestSummaryData = this.pendingSummaryData; + this.pendingSummaryData = undefined; + this.updatedDSCountSinceLastSummary = 0; + return; } /** diff --git a/packages/runtime/container-runtime/src/summary/index.ts b/packages/runtime/container-runtime/src/summary/index.ts index 872080c2111b..4af795551424 100644 --- a/packages/runtime/container-runtime/src/summary/index.ts +++ b/packages/runtime/container-runtime/src/summary/index.ts @@ -31,7 +31,6 @@ export { IFetchSnapshotResult, IRootSummarizerNode, IRootSummarizerNodeWithGC, - RefreshSummaryResult, } from "./summarizerNode"; export { IConnectableRuntime, diff --git a/packages/runtime/container-runtime/src/summary/summarizerNode/index.ts b/packages/runtime/container-runtime/src/summary/summarizerNode/index.ts index d1179b9ea808..9809a4496624 100644 --- a/packages/runtime/container-runtime/src/summary/summarizerNode/index.ts +++ b/packages/runtime/container-runtime/src/summary/summarizerNode/index.ts @@ -6,7 +6,6 @@ export { IFetchSnapshotResult, ISummarizerNodeRootContract, - RefreshSummaryResult, ValidateSummaryResult, } from "./summarizerNodeUtils"; export { IRootSummarizerNode, createRootSummarizerNode } from "./summarizerNode"; diff --git a/packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNode.ts b/packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNode.ts index 503779df5202..d2198ae73bf7 100644 --- a/packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNode.ts +++ b/packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNode.ts @@ -38,7 +38,6 @@ import { ISummarizerNodeRootContract, parseSummaryForSubtrees, parseSummaryTreeForSubtrees, - RefreshSummaryResult, SummaryNode, ValidateSummaryResult, } from "./summarizerNodeUtils"; @@ -365,27 +364,21 @@ export class SummarizerNode implements IRootSummarizerNode { /** * Refreshes the latest summary tracked by this node. If we have a pending summary for the given proposal handle, - * it becomes the latest summary. If the current summary is already ahead (e.g., loaded from a service summary), - * we skip the update. If the current summary is behind, then we do not refresh. + * it becomes the latest summary. If the current summary is already ahead, we skip the update. + * If the current summary is behind, then we do not refresh. * - * @returns A RefreshSummaryResult type which returns information based on the following three scenarios: - * - * 1. The latest summary was not updated. - * - * 2. The latest summary was updated and the summary corresponding to the params was being tracked. - * - * 3. The latest summary was updated but the summary corresponding to the params was not tracked. + * @returns true if the summary is tracked by this node, false otherwise. */ public async refreshLatestSummary( - proposalHandle: string | undefined, + proposalHandle: string, summaryRefSeq: number, - ): Promise { + ): Promise { const eventProps: { proposalHandle: string | undefined; summaryRefSeq: number; referenceSequenceNumber: number; - latestSummaryUpdated?: boolean; - wasSummaryTracked?: boolean; + isSummaryTracked?: boolean; + pendingSummaryFound?: boolean; } = { proposalHandle, summaryRefSeq, @@ -406,37 +399,22 @@ export class SummarizerNode implements IRootSummarizerNode { }); } - if (proposalHandle !== undefined) { - const maybeSummaryNode = this.pendingSummaries.get(proposalHandle); + let isSummaryTracked = false; + let isSummaryNewer = false; - if (maybeSummaryNode !== undefined) { - this.refreshLatestSummaryFromPending( - proposalHandle, - maybeSummaryNode.referenceSequenceNumber, - ); - eventProps.wasSummaryTracked = true; - eventProps.latestSummaryUpdated = true; - event.end(eventProps); - return { - latestSummaryUpdated: true, - wasSummaryTracked: true, - summaryRefSeq, - }; - } + if (summaryRefSeq > this.referenceSequenceNumber) { + isSummaryNewer = true; } - - // If the summary for which refresh is called is older than the latest tracked summary, ignore it. - if (this.referenceSequenceNumber >= summaryRefSeq) { - eventProps.latestSummaryUpdated = false; - event.end(eventProps); - return { latestSummaryUpdated: false }; + const maybeSummaryNode = this.pendingSummaries.get(proposalHandle); + if (maybeSummaryNode !== undefined) { + this.refreshLatestSummaryFromPending( + proposalHandle, + maybeSummaryNode.referenceSequenceNumber, + ); + isSummaryTracked = true; } - - // Note that we did not track this summary, but that the latest summary was updated. - eventProps.latestSummaryUpdated = true; - eventProps.wasSummaryTracked = false; - event.end(eventProps); - return { latestSummaryUpdated: true, wasSummaryTracked: false }; + event.end({ ...eventProps, isSummaryNewer, pendingSummaryFound: isSummaryTracked }); + return isSummaryTracked; }, { start: true, end: true, cancel: "error" }, ); diff --git a/packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNodeUtils.ts b/packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNodeUtils.ts index 99be77de4cad..6ba3cf47f41c 100644 --- a/packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNodeUtils.ts +++ b/packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNodeUtils.ts @@ -7,30 +7,6 @@ import { ITelemetryLoggerExt, TelemetryDataTag } from "@fluidframework/telemetry import { ISnapshotTree, ISummaryTree, SummaryObject } from "@fluidframework/protocol-definitions"; import { channelsTreeName, ISummaryTreeWithStats } from "@fluidframework/runtime-definitions"; -/** - * Return type of refreshSummaryAck function. There can be three different scenarios based on the passed params: - * - * 1. The latest summary was not updated. - * - * 2. The latest summary was updated and the summary corresponding to the params was tracked by this client. - * - * 3. The latest summary was updated but the summary corresponding to the params was not tracked. The client should - * close - */ -export type RefreshSummaryResult = - | { - latestSummaryUpdated: false; - } - | { - latestSummaryUpdated: true; - wasSummaryTracked: true; - summaryRefSeq: number; - } - | { - latestSummaryUpdated: true; - wasSummaryTracked: false; - }; - /** * Result of snapshot fetch during refreshing latest summary state. */ @@ -66,10 +42,7 @@ export interface ISummarizerNodeRootContract { validateSummary(): ValidateSummaryResult; completeSummary(proposalHandle: string, validate: boolean): void; clearSummary(): void; - refreshLatestSummary( - proposalHandle: string | undefined, - summaryRefSeq: number, - ): Promise; + refreshLatestSummary(proposalHandle: string, summaryRefSeq: number): Promise; } /** Path for nodes in a tree with escaped special characters */ diff --git a/packages/runtime/container-runtime/src/test/gc/garbageCollection.spec.ts b/packages/runtime/container-runtime/src/test/gc/garbageCollection.spec.ts index 704c399ab4d8..b0b8009d7dc1 100644 --- a/packages/runtime/container-runtime/src/test/gc/garbageCollection.spec.ts +++ b/packages/runtime/container-runtime/src/test/gc/garbageCollection.spec.ts @@ -53,7 +53,6 @@ import { dataStoreAttributesBlobName, IContainerRuntimeMetadata, metadataBlobName, - RefreshSummaryResult, } from "../../summary"; import { pkgVersion } from "../../packageVersion"; import { configProvider } from "./gcUnitTestHelpers"; @@ -1098,12 +1097,7 @@ describe("Garbage Collection Tests", () => { "Deleted nodes state should be a handle", ); - const refreshSummaryResult: RefreshSummaryResult = { - latestSummaryUpdated: true, - wasSummaryTracked: true, - summaryRefSeq: 0, - }; - await garbageCollector.refreshLatestSummary(refreshSummaryResult); + await garbageCollector.refreshLatestSummary(true /* isSummaryTracked */); // Run GC and summarize again. The whole GC summary should now be a summary handle. await garbageCollector.collectGarbage({}); @@ -1697,12 +1691,7 @@ describe("Garbage Collection Tests", () => { checkGCSummaryType(tree1, SummaryType.Tree, "first"); - await garbageCollector.refreshLatestSummary({ - wasSummaryTracked: true, - latestSummaryUpdated: true, - summaryRefSeq: 0, - }); - + await garbageCollector.refreshLatestSummary(true /* isSummaryTracked */); await garbageCollector.collectGarbage({}); const tree2 = garbageCollector.summarize(fullTree, trackState); diff --git a/packages/runtime/container-runtime/src/test/gc/gcSummaryStateTracker.spec.ts b/packages/runtime/container-runtime/src/test/gc/gcSummaryStateTracker.spec.ts index c17dc5b77b04..de6c42ff1174 100644 --- a/packages/runtime/container-runtime/src/test/gc/gcSummaryStateTracker.spec.ts +++ b/packages/runtime/container-runtime/src/test/gc/gcSummaryStateTracker.spec.ts @@ -14,7 +14,6 @@ import { IGarbageCollectionState, IGCStats, } from "../../gc"; -import { RefreshSummaryResult } from "../../summary"; type GCSummaryStateTrackerWithPrivates = Omit & { latestSummaryGCVersion: GCVersion; @@ -41,12 +40,7 @@ describe("GCSummaryStateTracker tests", () => { ); // After the first summary succeeds (refreshLatestSummary called), the state should not need reset. - const refreshSummaryResult: RefreshSummaryResult = { - latestSummaryUpdated: true, - wasSummaryTracked: true, - summaryRefSeq: 0, - }; - await tracker.refreshLatestSummary(refreshSummaryResult); + await tracker.refreshLatestSummary(true /* isSummaryTracked */); assert.equal( tracker.doesSummaryStateNeedReset, @@ -94,12 +88,7 @@ describe("GCSummaryStateTracker tests", () => { ); // After the first summary succeeds (refreshLatestSummary called), the state should not need reset. - const refreshSummaryResult: RefreshSummaryResult = { - latestSummaryUpdated: true, - wasSummaryTracked: true, - summaryRefSeq: 0, - }; - await tracker.refreshLatestSummary(refreshSummaryResult); + await tracker.refreshLatestSummary(true /* isSummaryTracked */); assert.equal( tracker.doesSummaryStateNeedReset, false, @@ -135,12 +124,7 @@ describe("GCSummaryStateTracker tests", () => { assert.equal(tracker.doesGCStateNeedReset, true, "Should need reset"); // After the first summary succeeds (refreshLatestSummary called), the state should not need reset. - const refreshSummaryResult: RefreshSummaryResult = { - latestSummaryUpdated: true, - wasSummaryTracked: true, - summaryRefSeq: 0, - }; - await tracker.refreshLatestSummary(refreshSummaryResult); + await tracker.refreshLatestSummary(true /* isSummaryTracked */); assert.equal( tracker.doesGCStateNeedReset, false, @@ -161,12 +145,7 @@ describe("GCSummaryStateTracker tests", () => { assert.equal(tracker.doesGCStateNeedReset, true, "Should need reset"); // After the first summary succeeds (refreshLatestSummary called), the state should not need reset. - const refreshSummaryResult: RefreshSummaryResult = { - latestSummaryUpdated: true, - wasSummaryTracked: true, - summaryRefSeq: 0, - }; - await tracker.refreshLatestSummary(refreshSummaryResult); + await tracker.refreshLatestSummary(true /* isSummaryTracked */); assert.equal( tracker.doesGCStateNeedReset, @@ -384,12 +363,8 @@ describe("GCSummaryStateTracker tests", () => { new Set(), [], ); - const refreshSummaryResult: RefreshSummaryResult = { - latestSummaryUpdated: true, - wasSummaryTracked: true, - summaryRefSeq: 0, - }; - await summaryStateTracker.refreshLatestSummary(refreshSummaryResult); + + await summaryStateTracker.refreshLatestSummary(true /* isSummaryTracked */); assert.strictEqual( summaryStateTracker.updatedDSCountSinceLastSummary, 0, diff --git a/packages/runtime/container-runtime/src/test/summarizerNode.spec.ts b/packages/runtime/container-runtime/src/test/summarizerNode.spec.ts index 0dfc28e0f328..2c8adf70042c 100644 --- a/packages/runtime/container-runtime/src/test/summarizerNode.spec.ts +++ b/packages/runtime/container-runtime/src/test/summarizerNode.spec.ts @@ -416,17 +416,13 @@ describe("Runtime", () => { }); describe("Refresh Latest Summary", () => { - it("Should refresh from tree when no proposal handle provided", async () => { - createRoot(); - const result = await rootNode.refreshLatestSummary(undefined, summaryRefSeq); - assert(result.latestSummaryUpdated === true, "should update"); - assert(result.wasSummaryTracked === false, "should not be tracked"); - }); - it("Should not refresh latest if already passed ref seq number", async () => { createRoot({ refSeq: summaryRefSeq }); - const result = await rootNode.refreshLatestSummary(undefined, summaryRefSeq); - assert(result.latestSummaryUpdated === false, "we already got this summary"); + const isSummaryTracked = await rootNode.refreshLatestSummary( + "test-handle", + summaryRefSeq, + ); + assert(!isSummaryTracked, "we already got this summary"); }); it("Should refresh from pending", async () => { @@ -437,12 +433,11 @@ describe("Runtime", () => { await rootNode.summarize(false); rootNode.completeSummary(proposalHandle, true /* validateSummary */); - const result = await rootNode.refreshLatestSummary( + const isSummaryTracked = await rootNode.refreshLatestSummary( proposalHandle, summaryRefSeq, ); - assert(result.latestSummaryUpdated === true, "should update"); - assert(result.wasSummaryTracked === true, "should be tracked"); + assert(isSummaryTracked, "should be tracked"); }); it("should fail refresh when summary is in progress", async () => { diff --git a/packages/runtime/container-runtime/src/test/summarizerNodeWithGc.spec.ts b/packages/runtime/container-runtime/src/test/summarizerNodeWithGc.spec.ts index e76492e0353b..5239b83642a2 100644 --- a/packages/runtime/container-runtime/src/test/summarizerNodeWithGc.spec.ts +++ b/packages/runtime/container-runtime/src/test/summarizerNodeWithGc.spec.ts @@ -414,9 +414,11 @@ describe("SummarizerNodeWithGC Tests", () => { await midNode?.summarize(false); rootNode.completeSummary("test-handle1", true /* validateSummary */); - let result = await rootNode.refreshLatestSummary("test-handle1", summaryRefSeq); - assert(result.latestSummaryUpdated === true, "should update"); - assert(result.wasSummaryTracked === true, "should be tracked"); + let isSummaryTracked = await rootNode.refreshLatestSummary( + "test-handle1", + summaryRefSeq, + ); + assert(isSummaryTracked, "should be tracked"); rootNode.startSummary(summaryRefSeq++, logger); rootNode.updateUsedRoutes([`/`, `/${ids[1]}`, `/${ids[1]}/${ids[2]}`]); @@ -429,9 +431,8 @@ describe("SummarizerNodeWithGC Tests", () => { // Create a new child node for which we will need to create a pending summary for. createLeaf({ type: CreateSummarizerNodeSource.Local }); - result = await rootNode.refreshLatestSummary("test-handle2", summaryRefSeq); - assert(result.latestSummaryUpdated === true, "should update"); - assert(result.wasSummaryTracked === true, "should be tracked"); + isSummaryTracked = await rootNode.refreshLatestSummary("test-handle2", summaryRefSeq); + assert(isSummaryTracked, "should be tracked"); const leafNodePath = `${ids[0]}/${ids[1]}/${ids[2]}`; const leafNodeLatestSummary = (leafNode as SummarizerNodeWithGC).latestSummary; assert.strictEqual( @@ -452,9 +453,11 @@ describe("SummarizerNodeWithGC Tests", () => { await midNode?.summarize(false); rootNode.completeSummary("test-handle1", true /* validateSummary */); - let result = await rootNode.refreshLatestSummary("test-handle1", summaryRefSeq); - assert(result.latestSummaryUpdated === true, "should update"); - assert(result.wasSummaryTracked === true, "should be tracked"); + let isSummaryTracked = await rootNode.refreshLatestSummary( + "test-handle1", + summaryRefSeq, + ); + assert(isSummaryTracked, "should be tracked"); rootNode.startSummary(summaryRefSeq++, logger); rootNode.updateUsedRoutes([""]); @@ -467,9 +470,8 @@ describe("SummarizerNodeWithGC Tests", () => { // Create a new child node for which we will need to create a pending summary for. createLeaf({ type: CreateSummarizerNodeSource.Local }); - result = await rootNode.refreshLatestSummary("test-handle2", summaryRefSeq); - assert(result.latestSummaryUpdated === true, "should update"); - assert(result.wasSummaryTracked === true, "should be tracked"); + isSummaryTracked = await rootNode.refreshLatestSummary("test-handle2", summaryRefSeq); + assert(isSummaryTracked, "should be tracked"); const leafNodePath = `${ids[0]}/${ids[1]}/${ids[2]}`; const leafNodeLatestSummary = (leafNode as SummarizerNodeWithGC).latestSummary; assert.strictEqual( From df6dd0e74dcaff0035cfaa8297f9fb3a0df6de0b Mon Sep 17 00:00:00 2001 From: Navin Agarwal Date: Fri, 1 Sep 2023 17:14:39 +0000 Subject: [PATCH 3/5] PR comments --- .../container-runtime/src/gc/garbageCollection.ts | 3 +-- .../src/gc/gcSummaryStateTracker.ts | 15 ++++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/runtime/container-runtime/src/gc/garbageCollection.ts b/packages/runtime/container-runtime/src/gc/garbageCollection.ts index ec38caa4f427..e17502eaf46c 100644 --- a/packages/runtime/container-runtime/src/gc/garbageCollection.ts +++ b/packages/runtime/container-runtime/src/gc/garbageCollection.ts @@ -842,8 +842,7 @@ export class GarbageCollector implements IGarbageCollector { } /** - * Called to refresh the latest summary state. This happens when either a pending summary is acked or a snapshot - * is downloaded and should be used to update the state. + * Called to refresh the latest summary state. This happens when either a pending summary is acked. */ public async refreshLatestSummary(isSummaryTracked: boolean): Promise { return this.summaryStateTracker.refreshLatestSummary(isSummaryTracked); diff --git a/packages/runtime/container-runtime/src/gc/gcSummaryStateTracker.ts b/packages/runtime/container-runtime/src/gc/gcSummaryStateTracker.ts index 8656c834496a..5121ae57c867 100644 --- a/packages/runtime/container-runtime/src/gc/gcSummaryStateTracker.ts +++ b/packages/runtime/container-runtime/src/gc/gcSummaryStateTracker.ts @@ -263,18 +263,19 @@ export class GCSummaryStateTracker { } /** - * Called to refresh the latest summary state. This happens when either a pending summary is acked or a snapshot - * is downloaded and should be used to update the state. + * Called to refresh the latest summary state. This happens when either a pending summary is acked. */ public async refreshLatestSummary(isSummaryTracked: boolean): Promise { + if (!isSummaryTracked) { + return; + } + // If the summary is tracked, this client is the one that generated it. So, update wasGCRunInLatestSummary. // Note that this has to be updated if GC did not run too. Otherwise, `gcStateNeedsReset` will always return - // true in scenarios where GC is disabled but enabled in the snapshot we loaded from. - if (isSummaryTracked) { - this.wasGCRunInLatestSummary = this.configs.shouldRunGC; - } + // true in scenarios where GC is currently disabled but enabled in the snapshot we loaded from. + this.wasGCRunInLatestSummary = this.configs.shouldRunGC; - if (!isSummaryTracked || !this.configs.shouldRunGC) { + if (!this.configs.shouldRunGC) { return; } From a34326fcaa9b7e1de12ec8c6e7b9bda51370086d Mon Sep 17 00:00:00 2001 From: Navin Agarwal Date: Fri, 1 Sep 2023 18:27:00 +0000 Subject: [PATCH 4/5] Add isSummaryNewer param --- .../container-runtime/src/containerRuntime.ts | 6 +++--- .../src/gc/garbageCollection.ts | 5 +++-- .../container-runtime/src/gc/gcDefinitions.ts | 8 ++++++-- .../src/gc/gcSummaryStateTracker.ts | 8 ++++---- .../container-runtime/src/summary/index.ts | 2 +- .../src/summary/summarizerNode/index.ts | 2 +- .../src/summary/summarizerNode/summarizerNode.ts | 5 +++-- .../summarizerNode/summarizerNodeUtils.ts | 16 +++++++++------- .../src/test/gc/garbageCollection.spec.ts | 10 ++++++++-- .../src/test/gc/gcSummaryStateTracker.spec.ts | 13 ++++++++----- 10 files changed, 46 insertions(+), 29 deletions(-) diff --git a/packages/runtime/container-runtime/src/containerRuntime.ts b/packages/runtime/container-runtime/src/containerRuntime.ts index 573d8dfff58b..4a891e1a4dd4 100644 --- a/packages/runtime/container-runtime/src/containerRuntime.ts +++ b/packages/runtime/container-runtime/src/containerRuntime.ts @@ -3654,7 +3654,7 @@ export class ContainerRuntime // proposalHandle is always passed from RunningSummarizer. assert(proposalHandle !== undefined, "proposalHandle should be available"); const readAndParseBlob = async (id: string) => readAndParse(this.storage, id); - const isSummaryTracked = await this.summarizerNode.refreshLatestSummary( + const result = await this.summarizerNode.refreshLatestSummary( proposalHandle, summaryRefSeq, ); @@ -3665,7 +3665,7 @@ export class ContainerRuntime * it needs to refresh its state. Today refresh is done by fetching the latest snapshot to update the cache * and then close as the current main client is likely to be re-elected as the parent summarizer again. */ - if (!isSummaryTracked && summaryRefSeq > this.summarizerNode.referenceSequenceNumber) { + if (!result.isSummaryTracked && result.isSummaryNewer) { const fetchResult = await this.fetchSnapshotFromStorage( summaryLogger, { @@ -3707,7 +3707,7 @@ export class ContainerRuntime } // Notify the garbage collector so it can update its latest summary state. - await this.garbageCollector.refreshLatestSummary(isSummaryTracked); + await this.garbageCollector.refreshLatestSummary(result); } /** diff --git a/packages/runtime/container-runtime/src/gc/garbageCollection.ts b/packages/runtime/container-runtime/src/gc/garbageCollection.ts index e17502eaf46c..b70b11f02305 100644 --- a/packages/runtime/container-runtime/src/gc/garbageCollection.ts +++ b/packages/runtime/container-runtime/src/gc/garbageCollection.ts @@ -29,6 +29,7 @@ import { RuntimeHeaders, } from "../containerRuntime"; import { ClientSessionExpiredError } from "../error"; +import { IRefreshSummaryResult } from "../summary"; import { generateGCConfigs } from "./gcConfigs"; import { GCNodeType, @@ -844,8 +845,8 @@ export class GarbageCollector implements IGarbageCollector { /** * Called to refresh the latest summary state. This happens when either a pending summary is acked. */ - public async refreshLatestSummary(isSummaryTracked: boolean): Promise { - return this.summaryStateTracker.refreshLatestSummary(isSummaryTracked); + public async refreshLatestSummary(result: IRefreshSummaryResult): Promise { + return this.summaryStateTracker.refreshLatestSummary(result); } /** diff --git a/packages/runtime/container-runtime/src/gc/gcDefinitions.ts b/packages/runtime/container-runtime/src/gc/gcDefinitions.ts index 4ededa61482d..6d9ee0e1a0e0 100644 --- a/packages/runtime/container-runtime/src/gc/gcDefinitions.ts +++ b/packages/runtime/container-runtime/src/gc/gcDefinitions.ts @@ -14,7 +14,11 @@ import { } from "@fluidframework/runtime-definitions"; import { ReadAndParseBlob } from "@fluidframework/runtime-utils"; import { ITelemetryLoggerExt } from "@fluidframework/telemetry-utils"; -import { IContainerRuntimeMetadata, ICreateContainerMetadata } from "../summary"; +import { + IContainerRuntimeMetadata, + ICreateContainerMetadata, + IRefreshSummaryResult, +} from "../summary"; export type GCVersion = number; @@ -224,7 +228,7 @@ export interface IGarbageCollector { /** Returns the GC details generated from the base snapshot. */ getBaseGCDetails(): Promise; /** Called when the latest summary of the system has been refreshed. */ - refreshLatestSummary(isSummaryTracked: boolean): Promise; + refreshLatestSummary(result: IRefreshSummaryResult): Promise; /** Called when a node is updated. Used to detect and log when an inactive node is changed or loaded. */ nodeUpdated( nodePath: string, diff --git a/packages/runtime/container-runtime/src/gc/gcSummaryStateTracker.ts b/packages/runtime/container-runtime/src/gc/gcSummaryStateTracker.ts index 5121ae57c867..fd1dbde79e66 100644 --- a/packages/runtime/container-runtime/src/gc/gcSummaryStateTracker.ts +++ b/packages/runtime/container-runtime/src/gc/gcSummaryStateTracker.ts @@ -13,10 +13,10 @@ import { ISummaryTreeWithStats, } from "@fluidframework/runtime-definitions"; import { mergeStats, SummaryTreeBuilder } from "@fluidframework/runtime-utils"; -import { GCVersion, IGCStats } from "./gcDefinitions"; +import { IRefreshSummaryResult } from "../summary"; +import { GCVersion, IGarbageCollectorConfigs, IGCStats } from "./gcDefinitions"; import { generateSortedGCState } from "./gcHelpers"; import { IGarbageCollectionSnapshotData, IGarbageCollectionState } from "./gcSummaryDefinitions"; -import { IGarbageCollectorConfigs } from "."; export const gcStateBlobKey = `${gcBlobPrefix}_root`; @@ -265,8 +265,8 @@ export class GCSummaryStateTracker { /** * Called to refresh the latest summary state. This happens when either a pending summary is acked. */ - public async refreshLatestSummary(isSummaryTracked: boolean): Promise { - if (!isSummaryTracked) { + public async refreshLatestSummary(result: IRefreshSummaryResult): Promise { + if (!result.isSummaryTracked) { return; } diff --git a/packages/runtime/container-runtime/src/summary/index.ts b/packages/runtime/container-runtime/src/summary/index.ts index 4af795551424..07029478127b 100644 --- a/packages/runtime/container-runtime/src/summary/index.ts +++ b/packages/runtime/container-runtime/src/summary/index.ts @@ -28,7 +28,7 @@ export { SummarizeHeuristicData, SummarizeHeuristicRunner } from "./summarizerHe export { createRootSummarizerNode, createRootSummarizerNodeWithGC, - IFetchSnapshotResult, + IRefreshSummaryResult, IRootSummarizerNode, IRootSummarizerNodeWithGC, } from "./summarizerNode"; diff --git a/packages/runtime/container-runtime/src/summary/summarizerNode/index.ts b/packages/runtime/container-runtime/src/summary/summarizerNode/index.ts index 9809a4496624..c5edd913cb21 100644 --- a/packages/runtime/container-runtime/src/summary/summarizerNode/index.ts +++ b/packages/runtime/container-runtime/src/summary/summarizerNode/index.ts @@ -4,7 +4,7 @@ */ export { - IFetchSnapshotResult, + IRefreshSummaryResult, ISummarizerNodeRootContract, ValidateSummaryResult, } from "./summarizerNodeUtils"; diff --git a/packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNode.ts b/packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNode.ts index d2198ae73bf7..38b2264c73b1 100644 --- a/packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNode.ts +++ b/packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNode.ts @@ -35,6 +35,7 @@ import { EscapedPath, ICreateChildDetails, IInitialSummary, + IRefreshSummaryResult, ISummarizerNodeRootContract, parseSummaryForSubtrees, parseSummaryTreeForSubtrees, @@ -372,7 +373,7 @@ export class SummarizerNode implements IRootSummarizerNode { public async refreshLatestSummary( proposalHandle: string, summaryRefSeq: number, - ): Promise { + ): Promise { const eventProps: { proposalHandle: string | undefined; summaryRefSeq: number; @@ -414,7 +415,7 @@ export class SummarizerNode implements IRootSummarizerNode { isSummaryTracked = true; } event.end({ ...eventProps, isSummaryNewer, pendingSummaryFound: isSummaryTracked }); - return isSummaryTracked; + return { isSummaryTracked, isSummaryNewer }; }, { start: true, end: true, cancel: "error" }, ); diff --git a/packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNodeUtils.ts b/packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNodeUtils.ts index 6ba3cf47f41c..52cd627f6f76 100644 --- a/packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNodeUtils.ts +++ b/packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNodeUtils.ts @@ -7,12 +7,11 @@ import { ITelemetryLoggerExt, TelemetryDataTag } from "@fluidframework/telemetry import { ISnapshotTree, ISummaryTree, SummaryObject } from "@fluidframework/protocol-definitions"; import { channelsTreeName, ISummaryTreeWithStats } from "@fluidframework/runtime-definitions"; -/** - * Result of snapshot fetch during refreshing latest summary state. - */ -export interface IFetchSnapshotResult { - snapshotTree: ISnapshotTree; - snapshotRefSeq: number; +export interface IRefreshSummaryResult { + /** Tells whether this summary is tracked by this client. */ + isSummaryTracked: boolean; + /** Tells whether this summary is newer than the latest one tracked by this client. */ + isSummaryNewer: boolean; } /** @@ -42,7 +41,10 @@ export interface ISummarizerNodeRootContract { validateSummary(): ValidateSummaryResult; completeSummary(proposalHandle: string, validate: boolean): void; clearSummary(): void; - refreshLatestSummary(proposalHandle: string, summaryRefSeq: number): Promise; + refreshLatestSummary( + proposalHandle: string, + summaryRefSeq: number, + ): Promise; } /** Path for nodes in a tree with escaped special characters */ diff --git a/packages/runtime/container-runtime/src/test/gc/garbageCollection.spec.ts b/packages/runtime/container-runtime/src/test/gc/garbageCollection.spec.ts index b0b8009d7dc1..ae2d20648826 100644 --- a/packages/runtime/container-runtime/src/test/gc/garbageCollection.spec.ts +++ b/packages/runtime/container-runtime/src/test/gc/garbageCollection.spec.ts @@ -1097,7 +1097,10 @@ describe("Garbage Collection Tests", () => { "Deleted nodes state should be a handle", ); - await garbageCollector.refreshLatestSummary(true /* isSummaryTracked */); + await garbageCollector.refreshLatestSummary({ + isSummaryTracked: true, + isSummaryNewer: true, + }); // Run GC and summarize again. The whole GC summary should now be a summary handle. await garbageCollector.collectGarbage({}); @@ -1691,7 +1694,10 @@ describe("Garbage Collection Tests", () => { checkGCSummaryType(tree1, SummaryType.Tree, "first"); - await garbageCollector.refreshLatestSummary(true /* isSummaryTracked */); + await garbageCollector.refreshLatestSummary({ + isSummaryTracked: true, + isSummaryNewer: true, + }); await garbageCollector.collectGarbage({}); const tree2 = garbageCollector.summarize(fullTree, trackState); diff --git a/packages/runtime/container-runtime/src/test/gc/gcSummaryStateTracker.spec.ts b/packages/runtime/container-runtime/src/test/gc/gcSummaryStateTracker.spec.ts index de6c42ff1174..d9b5461882cc 100644 --- a/packages/runtime/container-runtime/src/test/gc/gcSummaryStateTracker.spec.ts +++ b/packages/runtime/container-runtime/src/test/gc/gcSummaryStateTracker.spec.ts @@ -40,7 +40,7 @@ describe("GCSummaryStateTracker tests", () => { ); // After the first summary succeeds (refreshLatestSummary called), the state should not need reset. - await tracker.refreshLatestSummary(true /* isSummaryTracked */); + await tracker.refreshLatestSummary({ isSummaryTracked: true, isSummaryNewer: true }); assert.equal( tracker.doesSummaryStateNeedReset, @@ -88,7 +88,7 @@ describe("GCSummaryStateTracker tests", () => { ); // After the first summary succeeds (refreshLatestSummary called), the state should not need reset. - await tracker.refreshLatestSummary(true /* isSummaryTracked */); + await tracker.refreshLatestSummary({ isSummaryTracked: true, isSummaryNewer: true }); assert.equal( tracker.doesSummaryStateNeedReset, false, @@ -124,7 +124,7 @@ describe("GCSummaryStateTracker tests", () => { assert.equal(tracker.doesGCStateNeedReset, true, "Should need reset"); // After the first summary succeeds (refreshLatestSummary called), the state should not need reset. - await tracker.refreshLatestSummary(true /* isSummaryTracked */); + await tracker.refreshLatestSummary({ isSummaryTracked: true, isSummaryNewer: true }); assert.equal( tracker.doesGCStateNeedReset, false, @@ -145,7 +145,7 @@ describe("GCSummaryStateTracker tests", () => { assert.equal(tracker.doesGCStateNeedReset, true, "Should need reset"); // After the first summary succeeds (refreshLatestSummary called), the state should not need reset. - await tracker.refreshLatestSummary(true /* isSummaryTracked */); + await tracker.refreshLatestSummary({ isSummaryTracked: true, isSummaryNewer: true }); assert.equal( tracker.doesGCStateNeedReset, @@ -364,7 +364,10 @@ describe("GCSummaryStateTracker tests", () => { [], ); - await summaryStateTracker.refreshLatestSummary(true /* isSummaryTracked */); + await summaryStateTracker.refreshLatestSummary({ + isSummaryTracked: true, + isSummaryNewer: true, + }); assert.strictEqual( summaryStateTracker.updatedDSCountSinceLastSummary, 0, From fde2bbd0ae9a38e02c6ab65b0bee90acead20f83 Mon Sep 17 00:00:00 2001 From: Navin Agarwal Date: Fri, 1 Sep 2023 18:33:45 +0000 Subject: [PATCH 5/5] Update other tests --- .../src/test/summarizerNode.spec.ts | 9 ++++--- .../src/test/summarizerNodeWithGc.spec.ts | 26 +++++++++---------- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/packages/runtime/container-runtime/src/test/summarizerNode.spec.ts b/packages/runtime/container-runtime/src/test/summarizerNode.spec.ts index 2c8adf70042c..e8604ab0c6dc 100644 --- a/packages/runtime/container-runtime/src/test/summarizerNode.spec.ts +++ b/packages/runtime/container-runtime/src/test/summarizerNode.spec.ts @@ -418,11 +418,11 @@ describe("Runtime", () => { describe("Refresh Latest Summary", () => { it("Should not refresh latest if already passed ref seq number", async () => { createRoot({ refSeq: summaryRefSeq }); - const isSummaryTracked = await rootNode.refreshLatestSummary( + const result = await rootNode.refreshLatestSummary( "test-handle", summaryRefSeq, ); - assert(!isSummaryTracked, "we already got this summary"); + assert(!result.isSummaryTracked, "we already got this summary"); }); it("Should refresh from pending", async () => { @@ -433,11 +433,12 @@ describe("Runtime", () => { await rootNode.summarize(false); rootNode.completeSummary(proposalHandle, true /* validateSummary */); - const isSummaryTracked = await rootNode.refreshLatestSummary( + const result = await rootNode.refreshLatestSummary( proposalHandle, summaryRefSeq, ); - assert(isSummaryTracked, "should be tracked"); + assert(result.isSummaryTracked, "should be tracked"); + assert(result.isSummaryNewer === true, "should be newer"); }); it("should fail refresh when summary is in progress", async () => { diff --git a/packages/runtime/container-runtime/src/test/summarizerNodeWithGc.spec.ts b/packages/runtime/container-runtime/src/test/summarizerNodeWithGc.spec.ts index 5239b83642a2..dcaa7910f046 100644 --- a/packages/runtime/container-runtime/src/test/summarizerNodeWithGc.spec.ts +++ b/packages/runtime/container-runtime/src/test/summarizerNodeWithGc.spec.ts @@ -414,11 +414,9 @@ describe("SummarizerNodeWithGC Tests", () => { await midNode?.summarize(false); rootNode.completeSummary("test-handle1", true /* validateSummary */); - let isSummaryTracked = await rootNode.refreshLatestSummary( - "test-handle1", - summaryRefSeq, - ); - assert(isSummaryTracked, "should be tracked"); + let result = await rootNode.refreshLatestSummary("test-handle1", summaryRefSeq); + assert(result.isSummaryTracked, "should be tracked"); + assert(result.isSummaryNewer === true, "should be newer"); rootNode.startSummary(summaryRefSeq++, logger); rootNode.updateUsedRoutes([`/`, `/${ids[1]}`, `/${ids[1]}/${ids[2]}`]); @@ -431,8 +429,9 @@ describe("SummarizerNodeWithGC Tests", () => { // Create a new child node for which we will need to create a pending summary for. createLeaf({ type: CreateSummarizerNodeSource.Local }); - isSummaryTracked = await rootNode.refreshLatestSummary("test-handle2", summaryRefSeq); - assert(isSummaryTracked, "should be tracked"); + result = await rootNode.refreshLatestSummary("test-handle2", summaryRefSeq); + assert(result.isSummaryTracked, "should be tracked"); + assert(result.isSummaryNewer === true, "should be newer"); const leafNodePath = `${ids[0]}/${ids[1]}/${ids[2]}`; const leafNodeLatestSummary = (leafNode as SummarizerNodeWithGC).latestSummary; assert.strictEqual( @@ -453,11 +452,9 @@ describe("SummarizerNodeWithGC Tests", () => { await midNode?.summarize(false); rootNode.completeSummary("test-handle1", true /* validateSummary */); - let isSummaryTracked = await rootNode.refreshLatestSummary( - "test-handle1", - summaryRefSeq, - ); - assert(isSummaryTracked, "should be tracked"); + let result = await rootNode.refreshLatestSummary("test-handle1", summaryRefSeq); + assert(result.isSummaryTracked, "should be tracked"); + assert(result.isSummaryNewer === true, "should be newer"); rootNode.startSummary(summaryRefSeq++, logger); rootNode.updateUsedRoutes([""]); @@ -470,8 +467,9 @@ describe("SummarizerNodeWithGC Tests", () => { // Create a new child node for which we will need to create a pending summary for. createLeaf({ type: CreateSummarizerNodeSource.Local }); - isSummaryTracked = await rootNode.refreshLatestSummary("test-handle2", summaryRefSeq); - assert(isSummaryTracked, "should be tracked"); + result = await rootNode.refreshLatestSummary("test-handle2", summaryRefSeq); + assert(result.isSummaryTracked, "should be tracked"); + assert(result.isSummaryNewer === true, "should be newer"); const leafNodePath = `${ids[0]}/${ids[1]}/${ids[2]}`; const leafNodeLatestSummary = (leafNode as SummarizerNodeWithGC).latestSummary; assert.strictEqual(