metabase: `(*DB).IterateCoveredByTombstones` method isn't used #1523

New issue

Closed

opened 2024-11-25 12:57:57 +00:00 by a-savchuk · 4 comments

a-savchuk commented

2024-11-25 12:57:57 +00:00

Member

As I can see, the (*DB).IterateCoveredByTombstones method isn't used anywhere

$ grep -rI --files-with-matches IterateCoveredByTombstones
pkg/local_object_storage/metabase/iterators.go
pkg/local_object_storage/metabase/iterators_test.go

Should it be ~~removed~~ deprecated?

As I can see, the `(*DB).IterateCoveredByTombstones` method isn't used anywhere ```console $ grep -rI --files-with-matches IterateCoveredByTombstones pkg/local_object_storage/metabase/iterators.go pkg/local_object_storage/metabase/iterators_test.go ``` Should it be ~~removed~~ deprecated?

a-savchuk added the

triage

refactoring

labels 2024-11-25 12:57:57 +00:00

a-savchuk commented

2024-11-25 13:01:37 +00:00

Author

Member

![image_2024-11-25_16-00-36.png](/attachments/84924824-216f-47a1-88f0-5727d89e5cde)

image_2024-11-25_16-00-36.png

90 KiB

fyrchik commented

2024-11-25 13:23:19 +00:00

Owner

Let's also find where it was removed to have issue to link with.

fyrchik added

and removed

labels 2024-11-25 13:25:26 +00:00

a-savchuk was assigned by fyrchik

2024-11-26 15:44:56 +00:00

fyrchik commented

2024-11-26 15:45:14 +00:00

Owner

We can remove it immediately, no need to deprecate.

a-savchuk commented

2024-11-28 07:35:38 +00:00

Author

Member

Let's also find where it was removed to have issue to link with.

It became unused in 7799f8e4c

$ git log -G IterateCoveredByTombstones --oneline --name-only
059e9e88a [#373] metabase: Add metrics
pkg/local_object_storage/metabase/iterators.go
pkg/local_object_storage/metabase/iterators_test.go
1c30414a6 [#1454] Upgrade NeoFS SDK Go module with new IDs
pkg/local_object_storage/metabase/iterators.go
pkg/local_object_storage/metabase/iterators_test.go
7799f8e4c [#1318] engine: Change tombstone clear process
pkg/local_object_storage/shard/gc.go
303eb2e07 [#1176] metabase: Ignore locked objs in `IterateCoveredByTombstones`
pkg/local_object_storage/metabase/iterators_test.go
c1530dec5 [#1115] shard/gc: provide parsed addresses in `HandleExpiredTombstones`
pkg/local_object_storage/metabase/iterators.go
1667ec9e6 [#1131] *: Adopt SDK changes
pkg/local_object_storage/metabase/iterators.go
pkg/local_object_storage/metabase/iterators_test.go
pkg/local_object_storage/shard/gc.go
717f2beb4 [#378] shard: Collect expired tombstones in GC every epoch
pkg/local_object_storage/shard/gc.go
3d5169c4c [#378] metabase: Implement iterator over the objects under tombstones
pkg/local_object_storage/metabase/iterators.go
pkg/local_object_storage/metabase/iterators_test.go

$ git show -G IterateCoveredByTombstones 7799f8e4c
...
-func (s *Shard) HandleExpiredTombstones(tss map[string]*addressSDK.Address) {
-       inhume := make([]*addressSDK.Address, 0, len(tss))
-
-       // Collect all objects covered by the tombstones.
-
-       err := s.metaBase.IterateCoveredByTombstones(tss, func(addr *addressSDK.Address) error {
-               inhume = append(inhume, addr)
-               return nil
-       })
-       if err != nil {
-               s.log.Warn("iterator over expired objects failed",
-                       zap.String("error", err.Error()),
-               )
-
-               return
-       }
-
-       // Mark collected objects as garbage.
-
+func (s *Shard) HandleExpiredTombstones(tss []meta.TombstonedObject) {
+       // Mark tombstones as garbage.
        var pInhume meta.InhumePrm
 
-       pInhume.WithGCMark()
-
-       if len(inhume) > 0 {
-               // inhume objects
-               pInhume.WithAddresses(inhume...)
-
-               _, err = s.metaBase.Inhume(&pInhume)
-               if err != nil {
-                       s.log.Warn("could not inhume objects under the expired tombstone",
-                               zap.String("error", err.Error()),
-                       )
-
-                       return
-               }
+       tsAddrs := make([]*addressSDK.Address, 0, len(tss))
+       for _, ts := range tss {
+               tsAddrs = append(tsAddrs, ts.Tombstone())
        }
 
-       // Mark the tombstones as garbage.
-
-       inhume = inhume[:0]
-
-       for _, addr := range tss {
-               inhume = append(inhume, addr)
-       }
-
-       pInhume.WithAddresses(inhume...) // GC mark is already set above
+       pInhume.WithGCMark()
+       pInhume.WithAddresses(tsAddrs...)
 
        // inhume tombstones
-       _, err = s.metaBase.Inhume(&pInhume)
+       _, err := s.metaBase.Inhume(&pInhume)
        if err != nil {
                s.log.Warn("could not mark tombstones as garbage",
                        zap.String("error", err.Error()),
@@ -346,6 +356,13 @@ func (s *Shard) HandleExpiredTombstones(tss map[string]*addressSDK.Address) {
 
                return
        }
+
+       // drop just processed expired tombstones
+       // from graveyard
+       err = s.metaBase.DropGraves(tss)
+       if err != nil {
+               s.log.Warn("could not drop expired grave records", zap.Error(err))
+       }
 }

> Let's also find where it was removed to have issue to link with. It became unused in 7799f8e4c ```console $ git log -G IterateCoveredByTombstones --oneline --name-only 059e9e88a [#373] metabase: Add metrics pkg/local_object_storage/metabase/iterators.go pkg/local_object_storage/metabase/iterators_test.go 1c30414a6 [#1454] Upgrade NeoFS SDK Go module with new IDs pkg/local_object_storage/metabase/iterators.go pkg/local_object_storage/metabase/iterators_test.go 7799f8e4c [#1318] engine: Change tombstone clear process pkg/local_object_storage/shard/gc.go 303eb2e07 [#1176] metabase: Ignore locked objs in `IterateCoveredByTombstones` pkg/local_object_storage/metabase/iterators_test.go c1530dec5 [#1115] shard/gc: provide parsed addresses in `HandleExpiredTombstones` pkg/local_object_storage/metabase/iterators.go 1667ec9e6 [#1131] *: Adopt SDK changes pkg/local_object_storage/metabase/iterators.go pkg/local_object_storage/metabase/iterators_test.go pkg/local_object_storage/shard/gc.go 717f2beb4 [#378] shard: Collect expired tombstones in GC every epoch pkg/local_object_storage/shard/gc.go 3d5169c4c [#378] metabase: Implement iterator over the objects under tombstones pkg/local_object_storage/metabase/iterators.go pkg/local_object_storage/metabase/iterators_test.go $ git show -G IterateCoveredByTombstones 7799f8e4c ... -func (s *Shard) HandleExpiredTombstones(tss map[string]*addressSDK.Address) { - inhume := make([]*addressSDK.Address, 0, len(tss)) - - // Collect all objects covered by the tombstones. - - err := s.metaBase.IterateCoveredByTombstones(tss, func(addr *addressSDK.Address) error { - inhume = append(inhume, addr) - return nil - }) - if err != nil { - s.log.Warn("iterator over expired objects failed", - zap.String("error", err.Error()), - ) - - return - } - - // Mark collected objects as garbage. - +func (s *Shard) HandleExpiredTombstones(tss []meta.TombstonedObject) { + // Mark tombstones as garbage. var pInhume meta.InhumePrm - pInhume.WithGCMark() - - if len(inhume) > 0 { - // inhume objects - pInhume.WithAddresses(inhume...) - - _, err = s.metaBase.Inhume(&pInhume) - if err != nil { - s.log.Warn("could not inhume objects under the expired tombstone", - zap.String("error", err.Error()), - ) - - return - } + tsAddrs := make([]*addressSDK.Address, 0, len(tss)) + for _, ts := range tss { + tsAddrs = append(tsAddrs, ts.Tombstone()) } - // Mark the tombstones as garbage. - - inhume = inhume[:0] - - for _, addr := range tss { - inhume = append(inhume, addr) - } - - pInhume.WithAddresses(inhume...) // GC mark is already set above + pInhume.WithGCMark() + pInhume.WithAddresses(tsAddrs...) // inhume tombstones - _, err = s.metaBase.Inhume(&pInhume) + _, err := s.metaBase.Inhume(&pInhume) if err != nil { s.log.Warn("could not mark tombstones as garbage", zap.String("error", err.Error()), @@ -346,6 +356,13 @@ func (s *Shard) HandleExpiredTombstones(tss map[string]*addressSDK.Address) { return } + + // drop just processed expired tombstones + // from graveyard + err = s.metaBase.DropGraves(tss) + if err != nil { + s.log.Warn("could not drop expired grave records", zap.Error(err)) + } } ```

a-savchuk referenced this issue from a pull request that will close it,

2024-11-28 07:45:04 +00:00

metabase: Remove (*DB).IterateCoveredByTombstones #1526