GC: Tombstone for blobs#12880
Conversation
github-actions
Bot
added
area: runtime
github-actions
Bot
added
area: build
|
🔗 Found some broken links! 💔 Run a link check locally to find them. See linkcheck output |
github-actions
Bot
removed
area: dev experience
| // GC tombstones these blobs | ||
| if (tombstone) { | ||
| this.tombstonedBlobs.add(blobId); | ||
| continue; |
There was a problem hiding this comment.
You can remove the continue statements from both these blocks are there is no code after this.
There was a problem hiding this comment.
Or you can keep this continue and remove the else if block.
| if (tombstone) { | ||
| this.tombstonedBlobs.add(blobId); | ||
| continue; | ||
| } else if (this.redirectTable?.has(blobId)) { |
There was a problem hiding this comment.
This check is not needed actually. You can directly call this.redirectTable.delete(blobId). The code is this way because of some changes that happened with blob manager.
| const fluidHandleContext = defaultDataObject._context.containerRuntime.IFluidHandleContext; | ||
|
|
||
| // Handle requests for blob handle should fail! | ||
| const response = await fluidHandleContext.resolveHandle({ url: absolutePath }); |
There was a problem hiding this comment.
You should get the blob via its handle not via request / resolveHandle because that is what will happen in real scenarios.
This is fine to keep too but getting from handle is important as well.
There was a problem hiding this comment.
I'm going to have to create a handle or something because there's no way of getting the handle from the container as the blob doesn't exist.
There was a problem hiding this comment.
You already have a handle and can return it from summarizationWithUnreferencedBlobAfterTime.
There was a problem hiding this comment.
How? The blob handle that I have on the creation container is on a container that doesn't load the blob as tombstoned/unreferenced.
There was a problem hiding this comment.
Oh I see. I didn't look into detail and assumed that it's the same container.
| }); | ||
|
|
||
| // If this test starts failing due to runtime is closed errors try first adjusting `sweepTimeoutMs` above | ||
| itExpects("Handle request for tombstoned blobs only logs in summarizing container loaded after sweep timeout", |
There was a problem hiding this comment.
This test is also testing non-summarizer container. Also, its testing that with the ThrowOnTombstoneUsage set to false, there is no error thrown.
There was a problem hiding this comment.
You should add a third test case which validates that on summarizer container, no error is thrown even with the ThrowOnTombstoneUsage setting enabled.
There was a problem hiding this comment.
The summarizing container would close if we ThrowOnTombstoneUsage. So it's indirectly tested here.
There was a problem hiding this comment.
Sure but you should fix the test names to reflect what they are testing. Having an explicit test for summarizer container would be useful too. Also, summarizer container should not close (you said would).
There was a problem hiding this comment.
I'll add an extra test here, I originally thought adding another test would be redundant since it's already indirectly tested.
| import { delay, stringToBuffer } from "@fluidframework/common-utils"; | ||
| import { LoaderHeader } from "@fluidframework/container-definitions"; | ||
|
|
||
| describeNoCompat("GC tombstone blob tests", (getTestObjectProvider) => { |
There was a problem hiding this comment.
There tests won't be enough. Attachment blobs have different behavior based on whether they were loaded during detached or attached state. Also, blobs can be de-duped which adds to the mix.
See
|
Closing in favor of #13266 |
3 participants
Tombstone for GC Blobs
Done when