Fix: Simple object with lock and expiration time not removed after locks are expired #183
Labels
No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-node#183
Loading…
Reference in a new issue
No description provided.
Delete branch "dstepanov-yadro/frostfs-node:bug/OBJECT-2279-v2"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
dd05af5d07
to04a7623d2c
04a7623d2c
to7dde1d9b51
7dde1d9b51
to514b7212b0
WIP: Fix: Simple object with lock and expiration time not removed after locks are expiredto Fix: Simple object with lock and expiration time not removed after locks are expired@ -0,0 +10,4 @@
"github.com/stretchr/testify/require"
)
func TestDB_SelectExpired(t *testing.T) {
Because of how we store objects, I think it is worth to extend this test with a container containing multiple objects (some of them expire, others not).
fixed
@ -0,0 +37,4 @@
func getAddressSafe(o *object.Object) oid.Address {
cid, set := o.ContainerID()
if !set {
panic("container id required")
Given the recent PR,
require.True(t, set)
?fixed
@ -96,26 +101,30 @@ func (db *DB) Lock(cnr cid.ID, locker oid.ID, locked []oid.ID) error {
}
// FreeLockedBy unlocks all objects in DB which are locked by lockers.
Can we describe the meaning of the first return value in the comment?
fixed
@ -106,3 +111,2 @@
return db.boltDB.Update(func(tx *bbolt.Tx) error {
var err error
unlockedObjects := make([]oid.Address, 0)
Why not
nil
?fixed
@ -164,3 +176,2 @@
if len(keyLockers) == 1 {
// locker was all alone
err = bucketLockedContainer.Delete(k)
updates = append(updates, keyValue{
An idea for a linter, actually.
Not that it matters a lot, but can we justify accumulating (possibly big enough) slice in memory? Note, that it is a write transaction.
Have you considered working with
bucket.Cursor
instead ofForEach
?Not that it matters a lot, but can we justify accumulating (possibly big enough) slice in memory? Note, that it is a write transaction.
Have you considered working with
bucket.Cursor
instead ofForEach
?514b7212b0
to2340e0735d
2340e0735d
to23845a2fcb
23845a2fcb
to8e8d353743
@ -0,0 +47,4 @@
return err
}
for _, o := range append(expiredNeoFS, expiredSys...) {
Do we need an
append
just for iterating?fixed
8e8d353743
to1077adfb13
@ -540,6 +545,7 @@ func (s *Shard) HandleExpiredLocks(lockers []oid.Address) {
var pInhume meta.InhumePrm
pInhume.SetAddresses(lockers...)
pInhume.SetGCMark()
We can (should) replace
SetGCMark
withSetForceGCMark
, the former does nothing here.fixed
why? how did that work before then?
ForceGCMark
was (and is i guess) expected to be used by the control service only. since we are removing LOCKs (not locked) objects, another param should be responsible for it (SetLockObjectHandling
which is false by default).I don't think we need a lot of different
bool
parameters with same function.oh, i see. i would add a separate commit (that would also expand testing) for that then. it sound strange to me if no lock objects were removed but also no unit tests failed
it used to have a little bit different meaning before and currently it is useless, yes
1077adfb13
to17bc4a5216
Have you considered the opposite approach: DO issue GC marks for every expired object always but just skip them if they are locked and do not remove from the disk? When LOCK is dropped, another GC cycle would just remove them as usual. No need to filter objects, no need to allocate new maps etc.
Overall, looks good.
@ -0,0 +12,4 @@
"go.etcd.io/bbolt"
)
// SelectExpired return expired items from addresses.
took me some time to understand what it does: i expect it to be named smth like
Filter*
. we have someSelect
s already in the code and all they doSELECT
(mean going through all the objects they have and returning some subset)not critical, just FYI
fixed
@ -0,0 +15,4 @@
// SelectExpired return expired items from addresses.
// Address considered expired if metabase does contain information about expiration and
// expiration epoch is less than epoch.
func (db *DB) SelectExpired(ctx context.Context, epoch uint64, adresses []oid.Address) ([]oid.Address, error) {
s/adresses/addresses?
fixed
Collection of expired locks and expired objects work concurrently both on the same shard and on different shards. So it depends on what happens first: GC marks the regular object as expired or GC marks the lock object as expired.
17bc4a5216
toab32067152
Is that a problem? I meant the approach that adds a GC mark always (does not matter if an object is locked or not), but GC itself works only with GC marks that also do not have "LOCK" mark. I think that could simplified the code. Did not think about it deeply.