Skip to content
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.

Sign up for GitHub

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

Simplfied SummarizerNode:refreshLatestSummary logic #17106

Merged
merged 5 commits into from
Sep 1, 2023
Merged

Simplfied SummarizerNode:refreshLatestSummary logic #17106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5a7338e
Removed PendingSummaryNotFound telemetry log
agarwal-navin Aug 30, 2023
682762e
Update refresh params and result
agarwal-navin Aug 31, 2023
df6dd0e
PR comments
agarwal-navin Sep 1, 2023
a34326f
Add isSummaryNewer param
agarwal-navin Sep 1, 2023
fde2bbd
Update other tests
agarwal-navin Sep 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions packages/runtime/container-runtime/src/containerRuntime.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <T>(id: string) => readAndParse<T>(this.storage, id);
const result = 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 (!result.isSummaryTracked && result.isSummaryNewer) {
const fetchResult = await this.fetchSnapshotFromStorage(
summaryLogger,
{
Expand Down
7 changes: 3 additions & 4 deletions packages/runtime/container-runtime/src/gc/garbageCollection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import {
RuntimeHeaders,
} from "../containerRuntime";
import { ClientSessionExpiredError } from "../error";
import { RefreshSummaryResult } from "../summary";
import { IRefreshSummaryResult } from "../summary";
import { generateGCConfigs } from "./gcConfigs";
import {
GCNodeType,
Expand Down Expand Up @@ -843,10 +843,9 @@ 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(result: RefreshSummaryResult): Promise<void> {
public async refreshLatestSummary(result: IRefreshSummaryResult): Promise<void> {
return this.summaryStateTracker.refreshLatestSummary(result);
}

Expand Down
4 changes: 2 additions & 2 deletions packages/runtime/container-runtime/src/gc/gcDefinitions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { ITelemetryLoggerExt } from "@fluidframework/telemetry-utils";
import {
IContainerRuntimeMetadata,
ICreateContainerMetadata,
RefreshSummaryResult,
IRefreshSummaryResult,
} from "../summary";

export type GCVersion = number;
Expand Down Expand Up @@ -228,7 +228,7 @@ export interface IGarbageCollector {
/** Returns the GC details generated from the base snapshot. */
getBaseGCDetails(): Promise<IGarbageCollectionDetailsBase>;
/** Called when the latest summary of the system has been refreshed. */
refreshLatestSummary(result: RefreshSummaryResult): Promise<void>;
refreshLatestSummary(result: IRefreshSummaryResult): Promise<void>;
/** Called when a node is updated. Used to detect and log when an inactive node is changed or loaded. */
nodeUpdated(
nodePath: string,
Expand Down
39 changes: 17 additions & 22 deletions packages/runtime/container-runtime/src/gc/gcSummaryStateTracker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,10 @@ import {
ISummaryTreeWithStats,
} from "@fluidframework/runtime-definitions";
import { mergeStats, SummaryTreeBuilder } from "@fluidframework/runtime-utils";
import { RefreshSummaryResult } from "../summary";
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`;

Expand Down Expand Up @@ -264,31 +263,27 @@ 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(result: RefreshSummaryResult): Promise<void> {
// If the latest summary was updated and the summary was tracked, this client is the one that generated this
// summary. 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) {
this.wasGCRunInLatestSummary = this.configs.shouldRunGC;
}

if (!result.latestSummaryUpdated || !this.configs.shouldRunGC) {
public async refreshLatestSummary(result: IRefreshSummaryResult): Promise<void> {
if (!result.isSummaryTracked) {
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;
// 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 currently disabled but enabled in the snapshot we loaded from.
this.wasGCRunInLatestSummary = this.configs.shouldRunGC;

if (!this.configs.shouldRunGC) {
return;
}

this.latestSummaryGCVersion = this.configs.gcVersionInEffect;
this.latestSummaryData = this.pendingSummaryData;
this.pendingSummaryData = undefined;
this.updatedDSCountSinceLastSummary = 0;
return;
}

/**
Expand Down
3 changes: 1 addition & 2 deletions packages/runtime/container-runtime/src/summary/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,9 @@ export { SummarizeHeuristicData, SummarizeHeuristicRunner } from "./summarizerHe
export {
createRootSummarizerNode,
createRootSummarizerNodeWithGC,
IFetchSnapshotResult,
IRefreshSummaryResult,
IRootSummarizerNode,
IRootSummarizerNodeWithGC,
RefreshSummaryResult,
} from "./summarizerNode";
export {
IConnectableRuntime,
Expand Down
3 changes: 1 addition & 2 deletions packages/runtime/container-runtime/src/summary/summarizerNode/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@
*/

export {
IFetchSnapshotResult,
IRefreshSummaryResult,
ISummarizerNodeRootContract,
RefreshSummaryResult,
ValidateSummaryResult,
} from "./summarizerNodeUtils";
export { IRootSummarizerNode, createRootSummarizerNode } from "./summarizerNode";
Expand Down
74 changes: 21 additions & 53 deletions packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@ import {
EscapedPath,
ICreateChildDetails,
IInitialSummary,
IRefreshSummaryResult,
ISummarizerNodeRootContract,
parseSummaryForSubtrees,
parseSummaryTreeForSubtrees,
RefreshSummaryResult,
SummaryNode,
ValidateSummaryResult,
} from "./summarizerNodeUtils";
Expand Down Expand Up @@ -365,27 +365,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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of renaming this here and elsewhere to handleSummaryAck or onSummaryAck or something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reason why - Besides the obvious (we don't do any "refreshing", just tracking), the return value is typically called isSummaryTracked, but without all the context we have this reads kind of strangely:

const isSummaryTracked = refreshLatestSummary(...);

How about this:

const pendingSummaryFound = matchAckWithPendingSummary(...);

Or something similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather not do any such renames in this PR for 3 reasons:

  1. The PR is already big and it will become even bigger.
  2. It's will be a breaking change because the call from RunningSummarizer to ContainerRuntime is on a public interface.
  3. We do "refresh" the latest summary state. It's more of an update but refresh seems fine to me. I would be fine with calling it updateLatestSummaryState or something like that.

proposalHandle: string | undefined,
proposalHandle: string,
summaryRefSeq: number,
): Promise<RefreshSummaryResult> {
): Promise<IRefreshSummaryResult> {
const eventProps: {
proposalHandle: string | undefined;
summaryRefSeq: number;
referenceSequenceNumber: number;
latestSummaryUpdated?: boolean;
wasSummaryTracked?: boolean;
isSummaryTracked?: boolean;
pendingSummaryFound?: boolean;
} = {
proposalHandle,
summaryRefSeq,
Expand All @@ -406,48 +400,22 @@ export class SummarizerNode implements IRootSummarizerNode {
});
}

if (proposalHandle !== undefined) {
const maybeSummaryNode = this.pendingSummaries.get(proposalHandle);

if (maybeSummaryNode !== undefined) {
this.refreshLatestSummaryFromPending(
proposalHandle,
maybeSummaryNode.referenceSequenceNumber,
);
eventProps.wasSummaryTracked = true;
eventProps.latestSummaryUpdated = true;
event.end(eventProps);
return {
latestSummaryUpdated: true,
wasSummaryTracked: true,
summaryRefSeq,
};
}
let isSummaryTracked = false;
let isSummaryNewer = false;

const props = {
summaryRefSeq,
pendingSize: this.pendingSummaries.size ?? undefined,
};
this.logger.sendTelemetryEvent({
eventName: "PendingSummaryNotFound",
proposalHandle,
referenceSequenceNumber: this.referenceSequenceNumber,
details: JSON.stringify(props),
markfields marked this conversation as resolved.
Show resolved Hide resolved
});
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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observation: This case is no longer addressed by this function. It's up to the caller. This is probably fine, but I'd want us to think really carefully about the global state here, since this.referenceSequenceNumber can change throughout this flow.

I think it's fine because it only changes if we are tracking the incoming summary, in which case before his code would be unreachable anyway. And if we're not tracking, then it doesn't change, so it's fine for the caller to check.

Sound correct? Hopefully test coverage helps here too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird that this was >=. Not that they should be able to be equal but not the same (in which case it would be tracked). But you never know what could happen with Summaries!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is still there:

if (summaryRefSeq > this.referenceSequenceNumber) {
  isSummaryNewer = true;
}

isSummaryNewer is not returned because the caller has the information to gather this information. I could change it to return this value but I don't think its needed (infact I had it this way in my local repo and changed it).
IMO, the caller should decide what to do based on the return value isSummaryTracked. If the summary is tracked, the latest summary information (referenceSequenceNumber) will be updated and if not, it won't be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added isSummaryNewer to the return of this function and check that at the call site.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious Why you brought it back? I thought it was fine after I thought out loud about it. It was just a little hard to trace through the diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was on the fence about keeping it vs removing it from the point of view of not making significant changes in one go. With you comment, it seemed like it added some confusion so I added it back. We can remove it later if needed.

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, isSummaryNewer };
},
{ start: true, end: true, cancel: "error" },
);
Expand Down
39 changes: 7 additions & 32 deletions packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNodeUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,36 +7,11 @@ 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.
*/
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;
}

/**
Expand Down Expand Up @@ -67,9 +42,9 @@ export interface ISummarizerNodeRootContract {
completeSummary(proposalHandle: string, validate: boolean): void;
clearSummary(): void;
refreshLatestSummary(
proposalHandle: string | undefined,
proposalHandle: string,
summaryRefSeq: number,
): Promise<RefreshSummaryResult>;
): Promise<IRefreshSummaryResult>;
}

/** Path for nodes in a tree with escaped special characters */
Expand Down
17 changes: 6 additions & 11 deletions packages/runtime/container-runtime/src/test/gc/garbageCollection.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ import {
dataStoreAttributesBlobName,
IContainerRuntimeMetadata,
metadataBlobName,
RefreshSummaryResult,
} from "../../summary";
import { pkgVersion } from "../../packageVersion";
import { configProvider } from "./gcUnitTestHelpers";
Expand Down Expand Up @@ -1098,12 +1097,10 @@ 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({
isSummaryTracked: true,
isSummaryNewer: true,
});

// Run GC and summarize again. The whole GC summary should now be a summary handle.
await garbageCollector.collectGarbage({});
Expand Down Expand Up @@ -1698,11 +1695,9 @@ describe("Garbage Collection Tests", () => {
checkGCSummaryType(tree1, SummaryType.Tree, "first");

await garbageCollector.refreshLatestSummary({
wasSummaryTracked: true,
latestSummaryUpdated: true,
summaryRefSeq: 0,
isSummaryTracked: true,
isSummaryNewer: true,
});

await garbageCollector.collectGarbage({});
const tree2 = garbageCollector.summarize(fullTree, trackState);

Expand Down
Loading