policer: balance local shard data - #4170
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4170 +/- ##
==========================================
+ Coverage 31.03% 31.14% +0.10%
==========================================
Files 673 674 +1
Lines 40673 40716 +43
==========================================
+ Hits 12623 12681 +58
+ Misses 28050 28035 -15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
roman-khimov
left a comment
roman-khimov
left a comment
There was a problem hiding this comment.
If shard topology changes and this shard becomes the HRW-preferred target again before GC runs, relocation sees that the object already exists and does not clear the redundant mark. GC may then remove the current target copy.
This will be fixed by the next policer iteration. Too rare to care.
| return | ||
| } | ||
| if moved { | ||
| p.metrics.IncPolicerObjectRelocated(isEC) |
There was a problem hiding this comment.
Why separating EC and non-EC here? These are all physical objects.
| ListWithCursor(context.Context, uint32, *engine.Cursor, ...string) ([]objectcore.AddressWithAttributes, *engine.Cursor, error) | ||
| Delete(context.Context, oid.Address, engine.GarbageMark) error | ||
| DeleteRedundantCopies(context.Context, oid.Address, []string) error | ||
| RelocateObject(context.Context, oid.Address, []string) (bool, error) |
There was a problem hiding this comment.
It's more like OptimizeShardLocation to me. Relocate implies movement, but it's entirely optional here and in most cases this callback does nothing.
| return false, errShardNotFound | ||
| } | ||
|
|
||
| shards := e.sortedShards(addr.Object()) |
There was a problem hiding this comment.
Thought of avoiding getting this list again, but looks like it requires ListWithCursor changes. Shouldn't be performance-critical. At the same time we can optimize the scheme by checking placement in ListWithCursor and returning shard list only if there is something to fix there.
There was a problem hiding this comment.
Considered it, but that would sort shards for every listed object, including remotely redundant ones. Current code sorts only objects policer keeps locally and skips the target existence check for objects already listed only on their preferred shard.
| return false, apistatus.ObjectNotFound{} | ||
| } | ||
|
|
||
| err = e.putToShard(target, addr, obj, nil) |
There was a problem hiding this comment.
Do we have any log message for it?
| continue | ||
| } | ||
|
|
||
| if err = source.MarkGarbage(addr.Container(), []oid.ID{addr.Object()}, GarbageMarkRedundant); err != nil { |
62e9439 to
bed00fb
Compare
Relocate objects to their HRW-preferred local shard during policer scans. Mark old copies redundant only after a successful writing to the target. Closes #4147. Signed-off-by: Andrey Butusov <andrey@nspcc.io>
Closes #4147.
One edge case to consider: a copy marked with
GarbageMarkRedundantis still visible toExists,but remains queued for GC. If shard topology changes and this shard becomes the HRW-preferred target again before GC runs, relocation sees that the object already exists and does not clear the redundant mark. GC may then remove the current target copy.Should we handle this by adding a targeted operation similar to
ClearRedundantMarkfor the selected target shard, without reviving regular GC or tombstone marks?