-
Notifications
You must be signed in to change notification settings - Fork 549
Add read support for handles with pending payloads #24320
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
packages/runtime/container-runtime/src/blobManager/blobManager.ts:365
- Consider adding a default value for the 'placeholder' parameter (e.g., 'placeholder: boolean = false') to simplify calling this function in contexts where a placeholder is not explicitly needed.
public async getBlob(blobId: string, placeholder: boolean): Promise<ArrayBufferLike> {
packages/runtime/runtime-utils/src/remoteFluidObjectHandle.ts:38
- [nitpick] It would be helpful to add inline documentation for the 'placeholder' parameter to clarify its purpose and expected use across clients.
public readonly placeholder: boolean,
packages/runtime/container-runtime/src/summary/documentSchema.ts
Outdated
Show resolved
Hide resolved
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output
|
Adds: 1. DocumentSchema support for the new schema flag, such that these readable clients won't blow up once the future write-mode-capable clients turn the flag on 2. ContainerRuntime/BlobManager support for waiting on blobs to appear if their handles are tagged with `payloadPending` [AB#36250](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/36250)
@@ -1588,6 +1588,7 @@ describe("Runtime", () => { | |||
enableRuntimeIdCompressor: undefined, | |||
enableGroupedBatching: true, // Redundant, but makes the JSON.stringify yield the same result as the logs | |||
explicitSchemaControl: false, | |||
createBlobPayloadPending: false, // Redundant, but makes the JSON.stringify yield the same result as the logs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the read portion of placeholder handles for blobs (as prototyped in #24158). It specifically adds:
placeholder
Getting this up now for consideration but I'm still looking into an oddity in interactions between this read-only support and the full prototype version that I need to iron out - something weird happening when the full prototype tries to upgrade the document schema.Determined the oddities were an unrelated bug.AB#36250