engine: store lock object with locked at the same shard#3616
engine: store lock object with locked at the same shard#3616roman-khimov merged 1 commit intomasterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3616 +/- ##
==========================================
+ Coverage 26.58% 26.70% +0.11%
==========================================
Files 653 653
Lines 50006 50082 +76
==========================================
+ Hits 13296 13375 +79
- Misses 35661 35662 +1
+ Partials 1049 1045 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
This doesn't make me excited, but maybe it's OK as a temporary workaround till we solve #3476. In general a simple put is sufficient for new lock objects, they're designed exactly for this purpose (so Lock() can be removed eventually), but it's not that simple now.
Maybe a bit better one would be to do the same exists check for locked object in put handler. At least a bit more natural than putting in Lock().
carpawell
left a comment
There was a problem hiding this comment.
I would prefer to start thinking about placing helper objects based on their associated attribute, since we are already using 2.18 API. But if we need to solve smth fast and simple for this release, i am ok with this PR.
|
|
||
| putDone, exists, _ := e.putToShard(sh, i, pool, lockAddr, lockObj, objBin) | ||
| if putDone || exists { | ||
| return nil |
There was a problem hiding this comment.
are you sure we need to fast return? break below explains that if an object is root, we need to continue locking every shard
1423cac to
f3ed462
Compare
roman-khimov
left a comment
There was a problem hiding this comment.
Have you checked TS objects as well?
| } | ||
| } | ||
|
|
||
| return e.broadcastLockObject(obj, objBin) |
There was a problem hiding this comment.
do we even need e.lock() with this?
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the storage engine to broadcast lock objects to all shards instead of storing them on a single shard, ensuring lock objects are available everywhere in the system. This addresses an issue where expired locks were still locking objects.
- Implements broadcasting mechanism for lock objects across all shards
- Adds comprehensive test coverage for lock expiration scenarios
- Updates the changelog to reflect the fix for issue #3616
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/local_object_storage/engine/put.go | Adds broadcastLockObject function and integration into Put method for lock objects |
| pkg/local_object_storage/engine/lock_test.go | Adds test cases for split object and simple lock expiration scenarios |
| CHANGELOG.md | Updates the changelog entry to include the new issue number #3616 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| zap.Stringer("addr", addr)) | ||
| } | ||
| } else { | ||
| lastErr = fmt.Errorf("failed to put lock object on shard %s", sh.ID()) |
There was a problem hiding this comment.
The error message doesn't include the underlying cause. Consider wrapping the actual error from putToShard or providing more context about why the operation failed.
| lastErr = fmt.Errorf("failed to put lock object on shard %s", sh.ID()) | |
| lastErr = fmt.Errorf("failed to put lock object on shard %s (overloaded=%v)", sh.ID(), overloaded) |
f3ed462 to
cc3967c
Compare
| addr = object.AddressOf(obj) | ||
| ) | ||
|
|
||
| e.log.Debug("broadcasting %s object to all shards", |
There was a problem hiding this comment.
| e.log.Debug("broadcasting %s object to all shards", | |
| e.log.Debug("broadcasting object to all shards", |
| } | ||
|
|
||
| // broadcastObject stores object (lock or tombstone) on ALL shards to ensure it's available everywhere. | ||
| func (e *StorageEngine) broadcastObject(obj *objectSDK.Object, objBin []byte, objType objectSDK.Type) error { |
There was a problem hiding this comment.
getting objType from obj can prevent potential mistakes. We use it in text only, does not worth to be a prm
Case: the lock is located on one shard and its information is recorded in the metabucket of this shard, while the object being locked is located on another shard and its information is recorded in the lock bucket. As a result, when checking the second shard, the lock is located in the lock bucket, but it is not possible to find out its expiration date, and it is reported that it is locked. Therefore, store the lock object on all shards. The same situation may occur with the tombstone object, so broadcast it too. Signed-off-by: Andrey Butusov <andrey@nspcc.io>
cc3967c to
bef55cc
Compare
Is this solution appropriate? Should we also consider a put service that uses Lock?