Drop garbage bucket from metabase#3753
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3753 +/- ##
==========================================
- Coverage 26.49% 26.41% -0.09%
==========================================
Files 655 655
Lines 41414 41353 -61
==========================================
- Hits 10972 10922 -50
+ Misses 29434 29430 -4
+ Partials 1008 1001 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1052996 to
10e3506
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the garbage collection marking scheme in the metabase by migrating from a separate global garbage bucket to using metadata prefixes within container-specific buckets. This change simplifies the architecture and improves data locality.
- Migration added to move existing garbage marks from bucket
0x01to metadata prefix5during metabase version 8→9 upgrade - API signature changes for
IterateOverGarbageto require container ID and offset as object ID instead of address - Internal refactoring to remove
txparameter from helper functions likeobjectStatus,inGarbage, andobjectLocked
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/local_object_storage/metabase/version.go | Adds migration logic in migrateFrom8Version and moveGarbageToMeta to transfer garbage marks from old bucket to new metadata prefix |
| pkg/local_object_storage/metabase/version_test.go | Updates test to use new API and verify garbage marks via GetGarbage instead of directly checking bucket |
| pkg/local_object_storage/metabase/util.go | Removes garbageObjectsBucketName variable and updates prefix naming from garbageObjectsPrefix to unusedGarbageObjectsPrefix |
| pkg/local_object_storage/metabase/status.go | Removes garbageObjectsBucketName from bucket list and simplifies meta bucket null checks |
| pkg/local_object_storage/metabase/revive.go | Refactors ReviveObject to check garbage status via metadata cursor instead of separate bucket; removes getTombstoneByAssociatedObject helper |
| pkg/local_object_storage/metabase/put_test.go | Updates tests to use new IterateOverGarbage signature with container ID and object ID offset |
| pkg/local_object_storage/metabase/put.go | Changes garbage marking to use metaBkt.Put(mkGarbageKey(id), nil) instead of separate garbage bucket |
| pkg/local_object_storage/metabase/metadata.go | Adds metaPrefixGarbage constant and updates metadata deletion to remove garbage marks |
| pkg/local_object_storage/metabase/lock.go | Removes tx parameter from objectLocked function signature |
| pkg/local_object_storage/metabase/list.go | Refactors selectNFromBucket to remove garbage bucket parameter and use inGarbage with cursor; updates parseContainerIDWithPrefix return type |
| pkg/local_object_storage/metabase/iterators.go | Updates callers of parseContainerIDWithPrefix and objectLocked to match new signatures |
| pkg/local_object_storage/metabase/inhume.go | Removes garbage bucket operations and uses metadata bucket for garbage marking |
| pkg/local_object_storage/metabase/graveyard_test.go | Updates all tests to use new IterateOverGarbage signature requiring container ID; updates offset handling to use object ID |
| pkg/local_object_storage/metabase/graveyard.go | Replaces GarbageObject type and GarbageHandler with simpler func(oid.ID) callback; refactors iteration logic to use metadata cursor |
| pkg/local_object_storage/metabase/get.go | Removes listContainerObjects function (moved to graveyard.go as listGarbageObjects) and simplifies meta bucket checks |
| pkg/local_object_storage/metabase/exists.go | Replaces inGarbageWithKey with simpler inGarbage that checks metadata cursor using mkGarbageKey helper |
| pkg/local_object_storage/metabase/ec_test.go | Updates EC tests to use new IterateOverGarbage signature |
| pkg/local_object_storage/metabase/ec.go | Updates objectStatus call to remove tx parameter |
| pkg/local_object_storage/metabase/delete.go | Simplifies deletion by removing separate garbage bucket delete operation |
| pkg/local_object_storage/metabase/counter.go | Updates counter sync to use inGarbage without separate garbage bucket parameter |
| pkg/local_object_storage/metabase/control.go | Removes garbageObjectsBucketName from static buckets list |
| pkg/local_object_storage/metabase/containers.go | Simplifies container listing by using parseContainerIDWithPrefix helper consistently |
| pkg/local_object_storage/metabase/VERSION.md | Updates documentation to reflect removal of garbage bucket and addition of metadata prefix 5 |
| cmd/neofs-lens/internal/meta/list-garbage.go | Adds required --container flag and updates to use new IterateOverGarbage signature |
| CHANGELOG.md | Adds entry documenting the garbage marking scheme change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This simplifies the scheme by concentrating all of the object data in a single bucket which allows for better cursor reuse and requires less parameters in code. New keys are also just smaller since container is implied by the bucket. Signed-off-by: Roman Khimov <roman@nspcc.ru>
Allocation is likely to be optimized out, let's have simpler code for now. Meta is also the only real prefix+ID bucket left. Signed-off-by: Roman Khimov <roman@nspcc.ru>
Simplify some inner logic by checking the meta bucket first, practically nothing works without it, so handling this internally makes zero sense. Signed-off-by: Roman Khimov <roman@nspcc.ru>
10e3506 to
b5b0bcb
Compare
| metaBkt = tx.Bucket(bktKey) | ||
| ) | ||
| if metaBkt == nil { | ||
| log.Warn("no meta bucket found", zap.String("k", hex.EncodeToString(k))) |
There was a problem hiding this comment.
should we create this bucket? i think it is possible to have a GC mark in shard, but do not have a meta bucket cause no objects of this container were seen in this shard yet
There was a problem hiding this comment.
We have no objects from this container, garbage mark is irrelevant in this case, I think.
There was a problem hiding this comment.
well, ok, i just do not feel comfortable with this line
|
|
||
| var err = metaBkt.Put(objKey, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("put %s into %s: %w", hex.EncodeToString(objKey), hex.EncodeToString(bktKey), err) |
There was a problem hiding this comment.
why hex? base58 is more common for our IDs? at this line, we are sure sizes are ok, and IDs are expected to be correct
There was a problem hiding this comment.
It's "prefix+ID", so base58 won't be very helpful. This is one of the things that "should never happen".
There was a problem hiding this comment.
so base58 won't be very helpful
it may be resliced for logs
| return statusAvailable | ||
| } | ||
| var garbageMark = mkGarbageKey(id) | ||
| k, _ := metaCursor.Seek(garbageMark) |
There was a problem hiding this comment.
Because it creates a new cursor internally and we already have one, it's a bit more effective to seek. I've specifically not tried to optimize the code heavily now, but this one is too easy.
Ready to go.