[r11s] Fixing SummaryReader for wholeSummaryUpload: accessing map properties correctly #8864
Conversation
github-actions
bot
requested review from
tanviraumi,
anthony-murphy and
GaryWilber
hedasilv
requested review from
znewton,
tanviraumi,
anthony-murphy,
GaryWilber and
tianzhu007
and removed request for
anthony-murphy,
tanviraumi and
GaryWilber
server/routerlicious/packages/lambdas/src/scribe/summaryReader.ts
Outdated
Show resolved
Hide resolved
| if (!scribeBlobId && !deliBlobId && !opsBlobId) { | ||
| summaryReaderMetric.success(`Returning default summary when trying to read initial whole summary`); | ||
| return this.getDefaultSummaryState(); | ||
| } |
There was a problem hiding this comment.
We are making an assumption that either all three of them are present or none. This might be true today but I am not sure whether we should rely on it. Rather than doing that, we can only return default values for missing stuff. For example, "messages" in ILatestSummaryState comes from ".logtail" blob. So if logtail is missing, we only use the default value of messages field but use the rest from deli and scribe blob.
But I am okay if we check in this one for now and open a new one for filling specific field.
There was a problem hiding this comment.
Sure. I tried to accomplish that in the latest iteration. The part I'm not sure about now is fromSummary: true in the ILatestSummaryState. Now, it might not be 100% true that the data being read is from the actual summary. Is that a concern?
There was a problem hiding this comment.
I believe fromSummary needs to be true (https://github.com/microsoft/FluidFramework/blob/main/server/routerlicious/packages/lambdas/src/scribe/lambdaFactory.ts#L125). Otherwise we will return the whole DefaultScribe object. Double-check just to be confirmed.
There was a problem hiding this comment.
Correct - and that would also cause trouble to the lambda:
I will leave that as is then :)
…tate with default values
SummaryReaderis used to read summaries from storage. WhenSummaryReadertries toreadLastSummary()for the first time, that is, before Scribe has written any summaries, it will just find the blank/initial summary that Alfred uploaded. In that case, there is no.serviceProtocoltree, and the operation is expected to fail, thus returningSummaryReader'sgetDefaultSummaryState().However, we noticed that when WholeSummaryUpload was enabled, other calls to
readLastSummary()would also fail, but due to a different reason. That was becausenormalizedSummary.blobsis aMap<string, ArrayBuffer>and we were trying to access the map contents withmap[key]. That is not correct, as TS would interpret that as accessing the propertykeyof the object. As a result, values likeattributesContentwould beundefinedand that would cause errors later when usingbufferToString().The solution is to instead use
map.get(key)to access the map properties appropriately.