Repository navigation
Large GCS uploads no longer leave scratch objects behind - #2178
Michelle Au (msau42) merged 3 commits into
Conversation
|
ILLUM1N0X Taahir Ahmed (@ahmedtd) PTAL when you get a chance. Thank you! |
Max Smythe (maxsmythe)
added
kind/bug
|
Michelle Au (@msau42) for triage |
|
Tim Flannagan (@timflannagan) ktsoator (@ktsoator) PTAL when you get a chance. Thank you! |
|
Hi, Eric Curtin (@ericcurtin) :) Ah, I think you might have mentioned the wrong person! I'm not involved with maintaining this repository. Good luck with your PR though! |
|
ktsoator (@ktsoator) Sorry for the mix-up, and thank you for letting me know. Michelle Au (@msau42) Zoe Zhao (@zoez7) PTAL when you get a chance. Thank you! |
| for _, o := range objs { | ||
| if delErr := o.Delete(context.WithoutCancel(ctx)); delErr != nil && | ||
| !errors.Is(delErr, storage.ErrObjectNotExist) && err == nil { | ||
| err = fmt.Errorf("while removing upload part %q: %w", o.ObjectName(), delErr) |
There was a problem hiding this comment.
Update to: "while removing scratch object"?
| // Folding more than maxComposeSources parts goes through intermediate objects, and | ||
| // none may outlive the compose, including the ones the final compose consumes. | ||
| func TestComposeAllRemovesIntermediates(t *testing.T) { | ||
| client, bucket := emulatorClient(t) |
There was a problem hiding this comment.
🤖 should-fix 🟡 – emulatorClient skips whenever STORAGE_EMULATOR_HOST is unset, and CI never sets it, so this test never runs in CI. composeAll only sends Compose and Delete requests, so you can drive it against an httptest.NewServer (like TestPutObjectRetriesTransient429 in pkg/objectstorage/gcs_retry_test.go:72) and have it run on every go test.
| for _, s := range group { | ||
| _ = s.Delete(context.WithoutCancel(ctx)) | ||
| } | ||
| mids = append(mids, mid) |
There was a problem hiding this comment.
🤖 nit 🟢 – If Run(ctx) fails on the client after GCS committed mid (such as a timeout or dropped connection on the response), mid is not in mids and leaks. Append mid to mids before calling Run(ctx), matching how putComposite and PutSparseFile register parts before uploading. removeScratch already ignores storage.ErrObjectNotExist.
Signed-off-by: Eric Curtin <[email protected]>
|
Michelle Au (@msau42) Max Smythe (@maxsmythe) Addressed both notes (c9136c0), PTAL when you get a chance. Thank you! |
Fixes #2154
Large GCS uploads left their last compose intermediates behind. Now removed on return, and the duplicated cleanup is shared.
Test runs against fake-gcs-server.