From ca7d60ada8a2dedb940619f43c2b99554e4406c3 Mon Sep 17 00:00:00 2001 From: hedasilv Date: Mon, 24 Jan 2022 16:35:37 -0800 Subject: [PATCH 1/3] Fixing SummaryReader for wholeSummaryUpload: accessing map properties correctly --- .../lambdas/src/scribe/summaryReader.ts | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/server/routerlicious/packages/lambdas/src/scribe/summaryReader.ts b/server/routerlicious/packages/lambdas/src/scribe/summaryReader.ts index 93c05cad9802..f7d93efc9447 100644 --- a/server/routerlicious/packages/lambdas/src/scribe/summaryReader.ts +++ b/server/routerlicious/packages/lambdas/src/scribe/summaryReader.ts @@ -35,13 +35,17 @@ export class SummaryReader implements ISummaryReader { // Parse specific fields from the downloaded summary const attributesBlobId = normalizedSummary.snapshotTree.trees[".protocol"].blobs.attributes; - const attributesContent = normalizedSummary.blobs[attributesBlobId]; + const attributesContent = normalizedSummary.blobs.get(attributesBlobId); const scribeBlobId = normalizedSummary.snapshotTree.trees[".serviceProtocol"].blobs.scribe; - const scribeContent = normalizedSummary.blobs[scribeBlobId]; + const scribeContent = normalizedSummary.blobs.get(scribeBlobId); const deliBlobId = normalizedSummary.snapshotTree.trees[".serviceProtocol"].blobs.deli; - const deliContent = normalizedSummary.blobs[deliBlobId]; + const deliContent = normalizedSummary.blobs.get(deliBlobId); const opsBlobId = normalizedSummary.snapshotTree.trees[".logTail"].blobs.logTail; - const opsContent = normalizedSummary.blobs[opsBlobId]; + const opsContent = normalizedSummary.blobs.get(opsBlobId); + + if (!attributesContent || !scribeContent || !deliContent || !opsContent) { + throw new Error("Possible data corruption; summary data is missing important information."); + } const attributes = JSON.parse(bufferToString(attributesContent, "utf8")) as IDocumentAttributes; const scribe = bufferToString(scribeContent, "utf8"); @@ -67,8 +71,8 @@ export class SummaryReader implements ISummaryReader { messages, fromSummary: true, }; - } catch { - summaryReaderMetric.success(`Returning default summary`); + } catch (error: any) { + summaryReaderMetric.error(`Returning default summary`, error); return this.getDefaultSummaryState(); } } else { @@ -106,8 +110,8 @@ export class SummaryReader implements ISummaryReader { messages, fromSummary: true, }; - } catch { - summaryReaderMetric.success(`Returning default summary`); + } catch (error: any) { + summaryReaderMetric.error(`Returning default summary`, error); return this.getDefaultSummaryState(); } } From cc358103cdd0c71e2553cf363e72aebd0889be5a Mon Sep 17 00:00:00 2001 From: hedasilv Date: Tue, 25 Jan 2022 12:18:16 -0800 Subject: [PATCH 2/3] Improving error handling --- .../lambdas/src/scribe/summaryReader.ts | 47 +++++++++++++++---- 1 file changed, 37 insertions(+), 10 deletions(-) diff --git a/server/routerlicious/packages/lambdas/src/scribe/summaryReader.ts b/server/routerlicious/packages/lambdas/src/scribe/summaryReader.ts index f7d93efc9447..b7f4d85b45fc 100644 --- a/server/routerlicious/packages/lambdas/src/scribe/summaryReader.ts +++ b/server/routerlicious/packages/lambdas/src/scribe/summaryReader.ts @@ -33,18 +33,28 @@ export class SummaryReader implements ISummaryReader { const wholeFlatSummary = await this.summaryStorage.getSummary(existingRef.object.sha); const normalizedSummary = convertWholeFlatSummaryToSnapshotTreeAndBlobs(wholeFlatSummary); - // Parse specific fields from the downloaded summary + // Obtain IDs of specific fields from the downloaded summary const attributesBlobId = normalizedSummary.snapshotTree.trees[".protocol"].blobs.attributes; + const scribeBlobId = normalizedSummary.snapshotTree.trees[".serviceProtocol"]?.blobs?.scribe; + const deliBlobId = normalizedSummary.snapshotTree.trees[".serviceProtocol"]?.blobs?.deli; + const opsBlobId = normalizedSummary.snapshotTree.trees[".logTail"]?.blobs?.logTail; + + // The initial summary uploaded by Alfred has only .protocol. In other words, .serviceProtocol + // and .logTail would be both missing in that scenario and we should return the default summary + // state if that is the case. + if (!scribeBlobId && !deliBlobId && !opsBlobId) { + summaryReaderMetric.success(`Returning default summary when trying to read initial whole summary`); + return this.getDefaultSummaryState(); + } + + // Parse specific fields from the downloaded summary const attributesContent = normalizedSummary.blobs.get(attributesBlobId); - const scribeBlobId = normalizedSummary.snapshotTree.trees[".serviceProtocol"].blobs.scribe; const scribeContent = normalizedSummary.blobs.get(scribeBlobId); - const deliBlobId = normalizedSummary.snapshotTree.trees[".serviceProtocol"].blobs.deli; const deliContent = normalizedSummary.blobs.get(deliBlobId); - const opsBlobId = normalizedSummary.snapshotTree.trees[".logTail"].blobs.logTail; const opsContent = normalizedSummary.blobs.get(opsBlobId); if (!attributesContent || !scribeContent || !deliContent || !opsContent) { - throw new Error("Possible data corruption; summary data is missing important information."); + throw new Error("Possible data corruption; whole summary data is missing important information"); } const attributes = JSON.parse(bufferToString(attributesContent, "utf8")) as IDocumentAttributes; @@ -72,7 +82,7 @@ export class SummaryReader implements ISummaryReader { fromSummary: true, }; } catch (error: any) { - summaryReaderMetric.error(`Returning default summary`, error); + summaryReaderMetric.error(`Returning default summary due to error when reading whole summary`, error); return this.getDefaultSummaryState(); } } else { @@ -80,10 +90,27 @@ export class SummaryReader implements ISummaryReader { const existingRef = await this.summaryStorage.getRef(encodeURIComponent(this.documentId)); const [attributesContent, scribeContent, deliContent, opsContent] = await Promise.all([ this.summaryStorage.getContent(existingRef.object.sha, ".protocol/attributes"), - this.summaryStorage.getContent(existingRef.object.sha, ".serviceProtocol/scribe"), - this.summaryStorage.getContent(existingRef.object.sha, ".serviceProtocol/deli"), - this.summaryStorage.getContent(existingRef.object.sha, ".logTail/logTail"), + this.summaryStorage.getContent(existingRef.object.sha, ".serviceProtocol/scribe") + .catch(() => undefined), + this.summaryStorage.getContent(existingRef.object.sha, ".serviceProtocol/deli") + .catch(() => undefined), + this.summaryStorage.getContent(existingRef.object.sha, ".logTail/logTail") + .catch(() => undefined), ]); + + // The initial summary uploaded by Alfred has only .protocol. In other words, .serviceProtocol + // and .logTail would be both missing in that scenario and we should return the default summary + // state if that is the case. + if (!scribeContent && !deliContent && !opsContent) { + summaryReaderMetric.success(`Returning default summary when trying to read initial summary`); + return this.getDefaultSummaryState(); + } + + // If only part of .serviceProtocol or .logTail are missing, then it means we have an error. + if (!scribeContent || !deliContent || !opsContent) { + throw new Error("Possible data corruption; summary data is missing important information"); + } + const attributes = JSON.parse( toUtf8(attributesContent.content, attributesContent.encoding)) as IDocumentAttributes; const scribe = toUtf8(scribeContent.content, scribeContent.encoding); @@ -111,7 +138,7 @@ export class SummaryReader implements ISummaryReader { fromSummary: true, }; } catch (error: any) { - summaryReaderMetric.error(`Returning default summary`, error); + summaryReaderMetric.error(`Returning default summary due to error when reading summary`, error); return this.getDefaultSummaryState(); } } From 8930d403a45b18b9dbda1830a1a1aaaec9a9eef8 Mon Sep 17 00:00:00 2001 From: hedasilv Date: Tue, 25 Jan 2022 14:45:39 -0800 Subject: [PATCH 3/3] Updating summaryReader to replace only missing parts of ILastSummaryState with default values --- .../lambdas/src/scribe/lambdaFactory.ts | 2 +- .../lambdas/src/scribe/summaryReader.ts | 119 +++++++++++------- .../memory-orderer/src/localOrderer.ts | 2 +- 3 files changed, 78 insertions(+), 45 deletions(-) diff --git a/server/routerlicious/packages/lambdas/src/scribe/lambdaFactory.ts b/server/routerlicious/packages/lambdas/src/scribe/lambdaFactory.ts index 46bb1a6a77ce..ab680eed294a 100644 --- a/server/routerlicious/packages/lambdas/src/scribe/lambdaFactory.ts +++ b/server/routerlicious/packages/lambdas/src/scribe/lambdaFactory.ts @@ -85,7 +85,7 @@ export class ScribeLambdaFactory extends EventEmitter implements IPartitionLambd const tenant = await this.tenantManager.getTenant(tenantId, documentId); gitManager = tenant.gitManager; - summaryReader = new SummaryReader(documentId, gitManager, this.enableWholeSummaryUpload); + summaryReader = new SummaryReader(tenantId, documentId, gitManager, this.enableWholeSummaryUpload); [latestSummary, document] = await Promise.all([ summaryReader.readLastSummary(), this.documentCollection.findOne({ documentId, tenantId }), diff --git a/server/routerlicious/packages/lambdas/src/scribe/summaryReader.ts b/server/routerlicious/packages/lambdas/src/scribe/summaryReader.ts index b7f4d85b45fc..2f224aaf0135 100644 --- a/server/routerlicious/packages/lambdas/src/scribe/summaryReader.ts +++ b/server/routerlicious/packages/lambdas/src/scribe/summaryReader.ts @@ -7,18 +7,26 @@ import { bufferToString, toUtf8 } from "@fluidframework/common-utils"; import { IDocumentAttributes, ISequencedDocumentMessage } from "@fluidframework/protocol-definitions"; import { convertWholeFlatSummaryToSnapshotTreeAndBlobs, IGitManager } from "@fluidframework/server-services-client"; import { IDeliState } from "@fluidframework/server-services-core"; -import { CommonProperties, LumberEventName, Lumberjack } from "@fluidframework/server-services-telemetry"; +import { + CommonProperties, + getLumberBaseProperties, + LumberEventName, + Lumberjack, +} from "@fluidframework/server-services-telemetry"; import { ILatestSummaryState, ISummaryReader } from "./interfaces"; /** * Git specific implementation of ISummaryReader */ export class SummaryReader implements ISummaryReader { + private readonly lumberProperties: Record; constructor( + private readonly tenantId: string, private readonly documentId: string, private readonly summaryStorage: IGitManager, private readonly enableWholeSummaryUpload: boolean, ) { + this.lumberProperties = getLumberBaseProperties(this.documentId, this.tenantId); } /** @@ -27,6 +35,8 @@ export class SummaryReader implements ISummaryReader { */ public async readLastSummary(): Promise { const summaryReaderMetric = Lumberjack.newLumberMetric(LumberEventName.SummaryReader); + summaryReaderMetric.setProperties(this.lumberProperties); + if (this.enableWholeSummaryUpload) { try { const existingRef = await this.summaryStorage.getRef(encodeURIComponent(this.documentId)); @@ -34,34 +44,28 @@ export class SummaryReader implements ISummaryReader { const normalizedSummary = convertWholeFlatSummaryToSnapshotTreeAndBlobs(wholeFlatSummary); // Obtain IDs of specific fields from the downloaded summary - const attributesBlobId = normalizedSummary.snapshotTree.trees[".protocol"].blobs.attributes; + const attributesBlobId = normalizedSummary.snapshotTree.trees[".protocol"]?.blobs?.attributes; const scribeBlobId = normalizedSummary.snapshotTree.trees[".serviceProtocol"]?.blobs?.scribe; const deliBlobId = normalizedSummary.snapshotTree.trees[".serviceProtocol"]?.blobs?.deli; const opsBlobId = normalizedSummary.snapshotTree.trees[".logTail"]?.blobs?.logTail; - // The initial summary uploaded by Alfred has only .protocol. In other words, .serviceProtocol - // and .logTail would be both missing in that scenario and we should return the default summary - // state if that is the case. - if (!scribeBlobId && !deliBlobId && !opsBlobId) { - summaryReaderMetric.success(`Returning default summary when trying to read initial whole summary`); - return this.getDefaultSummaryState(); - } - // Parse specific fields from the downloaded summary - const attributesContent = normalizedSummary.blobs.get(attributesBlobId); - const scribeContent = normalizedSummary.blobs.get(scribeBlobId); - const deliContent = normalizedSummary.blobs.get(deliBlobId); - const opsContent = normalizedSummary.blobs.get(opsBlobId); - - if (!attributesContent || !scribeContent || !deliContent || !opsContent) { - throw new Error("Possible data corruption; whole summary data is missing important information"); - } - - const attributes = JSON.parse(bufferToString(attributesContent, "utf8")) as IDocumentAttributes; - const scribe = bufferToString(scribeContent, "utf8"); - const deli = JSON.parse(bufferToString(deliContent, "utf8")) as IDeliState; + const attributesContent = attributesBlobId ? normalizedSummary.blobs.get(attributesBlobId) : undefined; + const scribeContent = scribeBlobId ? normalizedSummary.blobs.get(scribeBlobId) : undefined; + const deliContent = deliBlobId ? normalizedSummary.blobs.get(deliBlobId) : undefined; + const opsContent = opsBlobId ? normalizedSummary.blobs.get(opsBlobId) : undefined; + + const attributes = attributesContent ? + JSON.parse(bufferToString(attributesContent, "utf8")) as IDocumentAttributes : + this.getDefaultAttributes(); + const scribe = scribeContent ? bufferToString(scribeContent, "utf8") : this.getDefaultScribe(); + const deli = deliContent ? + JSON.parse(bufferToString(deliContent, "utf8")) as IDeliState : + this.getDefaultDeli(); const term = deli.term; - const messages = JSON.parse(bufferToString(opsContent, "utf8")) as ISequencedDocumentMessage[]; + const messages = opsContent ? + JSON.parse(bufferToString(opsContent, "utf8")) as ISequencedDocumentMessage[] : + this.getDefaultMesages(); summaryReaderMetric.setProperties({ [CommonProperties.minLogtailSequenceNumber]: Math.min(...messages.map( @@ -89,7 +93,8 @@ export class SummaryReader implements ISummaryReader { try { const existingRef = await this.summaryStorage.getRef(encodeURIComponent(this.documentId)); const [attributesContent, scribeContent, deliContent, opsContent] = await Promise.all([ - this.summaryStorage.getContent(existingRef.object.sha, ".protocol/attributes"), + this.summaryStorage.getContent(existingRef.object.sha, ".protocol/attributes") + .catch(() => undefined), this.summaryStorage.getContent(existingRef.object.sha, ".serviceProtocol/scribe") .catch(() => undefined), this.summaryStorage.getContent(existingRef.object.sha, ".serviceProtocol/deli") @@ -98,26 +103,19 @@ export class SummaryReader implements ISummaryReader { .catch(() => undefined), ]); - // The initial summary uploaded by Alfred has only .protocol. In other words, .serviceProtocol - // and .logTail would be both missing in that scenario and we should return the default summary - // state if that is the case. - if (!scribeContent && !deliContent && !opsContent) { - summaryReaderMetric.success(`Returning default summary when trying to read initial summary`); - return this.getDefaultSummaryState(); - } - - // If only part of .serviceProtocol or .logTail are missing, then it means we have an error. - if (!scribeContent || !deliContent || !opsContent) { - throw new Error("Possible data corruption; summary data is missing important information"); - } - - const attributes = JSON.parse( - toUtf8(attributesContent.content, attributesContent.encoding)) as IDocumentAttributes; - const scribe = toUtf8(scribeContent.content, scribeContent.encoding); - const deli = JSON.parse(toUtf8(deliContent.content, deliContent.encoding)) as IDeliState; + const attributes = attributesContent ? + JSON.parse(toUtf8(attributesContent.content, attributesContent.encoding)) as IDocumentAttributes : + this.getDefaultAttributes(); + const scribe = scribeContent ? + toUtf8(scribeContent.content, scribeContent.encoding) : + this.getDefaultScribe(); + const deli = deliContent ? + JSON.parse(toUtf8(deliContent.content, deliContent.encoding)) as IDeliState : + this.getDefaultDeli(); const term = deli.term; - const messages = JSON.parse( - toUtf8(opsContent.content, opsContent.encoding)) as ISequencedDocumentMessage[]; + const messages = opsContent ? + JSON.parse(toUtf8(opsContent.content, opsContent.encoding)) as ISequencedDocumentMessage[] : + this.getDefaultMesages(); summaryReaderMetric.setProperties({ [CommonProperties.minLogtailSequenceNumber]: Math.min(...messages.map( @@ -153,4 +151,39 @@ export class SummaryReader implements ISummaryReader { fromSummary: false, }; } + + private getDefaultAttributes(): IDocumentAttributes { + Lumberjack.info("Using default attributes when reading summary.", this.lumberProperties); + return { + sequenceNumber: 0, + minimumSequenceNumber: 0, + term: undefined, + }; + } + + private getDefaultScribe(): string { + Lumberjack.info("Using default scribe when reading summary.", this.lumberProperties); + return ""; + } + + private getDefaultDeli(): IDeliState { + Lumberjack.info("Using default deli state when reading summary.", this.lumberProperties); + return { + clients: undefined, + durableSequenceNumber: 0, + logOffset: 0, + sequenceNumber: 0, + expHash1: "", + epoch: 0, + term: 1, + lastSentMSN: undefined, + nackMessages: undefined, + successfullyStartedLambdas: [], + }; + } + + private getDefaultMesages(): ISequencedDocumentMessage[] { + Lumberjack.info("Using default messages when reading summary.", this.lumberProperties); + return []; + } } diff --git a/server/routerlicious/packages/memory-orderer/src/localOrderer.ts b/server/routerlicious/packages/memory-orderer/src/localOrderer.ts index f5db1f159c3a..f75240fdec39 100644 --- a/server/routerlicious/packages/memory-orderer/src/localOrderer.ts +++ b/server/routerlicious/packages/memory-orderer/src/localOrderer.ts @@ -339,7 +339,7 @@ export class LocalOrderer implements IOrderer { this.gitManager, scribeMessagesCollection, false); - const summaryReader = new SummaryReader(this.documentId, this.gitManager, false); + const summaryReader = new SummaryReader(this.tenantId, this.documentId, this.gitManager, false); const checkpointManager = new CheckpointManager( context, this.tenantId,