Fix: Simple object with lock and expiration time not removed after locks are expired #183

Merged
fyrchik merged 1 commit from dstepanov-yadro/frostfs-node:bug/OBJECT-2279-v2 into master 2023-07-26 21:07:57 +00:00
  1. After removing the locks, the unblocked expired objects are inhumed.
  2. Fixed the logic of removing locks from objects: the lock was not removed in some cases.
1. After removing the locks, the unblocked expired objects are inhumed. 2. Fixed the logic of removing locks from objects: the lock was not removed in some cases.
dstepanov-yadro force-pushed bug/OBJECT-2279-v2 from dd05af5d07 to 04a7623d2c 2023-03-28 15:54:07 +00:00 Compare
dstepanov-yadro force-pushed bug/OBJECT-2279-v2 from 04a7623d2c to 7dde1d9b51 2023-03-29 08:06:27 +00:00 Compare
dstepanov-yadro force-pushed bug/OBJECT-2279-v2 from 7dde1d9b51 to 514b7212b0 2023-03-29 08:13:49 +00:00 Compare
dstepanov-yadro changed title from WIP: Fix: Simple object with lock and expiration time not removed after locks are expired to Fix: Simple object with lock and expiration time not removed after locks are expired 2023-03-29 08:14:06 +00:00
dstepanov-yadro requested review from storage-core-committers 2023-03-29 08:14:13 +00:00
dstepanov-yadro requested review from storage-core-developers 2023-03-29 08:14:13 +00:00
dstepanov-yadro added the
bug
frostfs-node
labels 2023-03-29 08:14:47 +00:00
fyrchik reviewed 2023-03-29 11:01:13 +00:00
@ -0,0 +10,4 @@
"github.com/stretchr/testify/require"
)
func TestDB_SelectExpired(t *testing.T) {
Owner

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).

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).
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
@ -0,0 +37,4 @@
func getAddressSafe(o *object.Object) oid.Address {
cid, set := o.ContainerID()
if !set {
panic("container id required")
Owner

Given the recent PR, require.True(t, set)?

Given the recent PR, `require.True(t, set)`?
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
@ -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.
Owner

Can we describe the meaning of the first return value in the comment?

Can we describe the meaning of the first return value in the comment?
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
@ -106,3 +111,2 @@
return db.boltDB.Update(func(tx *bbolt.Tx) error {
var err error
unlockedObjects := make([]oid.Address, 0)
Owner

Why not nil?

Why not `nil`?
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
@ -164,3 +176,2 @@
if len(keyLockers) == 1 {
// locker was all alone
err = bucketLockedContainer.Delete(k)
updates = append(updates, keyValue{
Owner

An idea for a linter, actually.

An idea for a linter, actually.
Owner

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 of ForEach?

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 of `ForEach`?
Owner

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 of ForEach?

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 of `ForEach`?
fyrchik marked this conversation as resolved
dstepanov-yadro force-pushed bug/OBJECT-2279-v2 from 514b7212b0 to 2340e0735d 2023-03-29 11:21:18 +00:00 Compare
dstepanov-yadro force-pushed bug/OBJECT-2279-v2 from 2340e0735d to 23845a2fcb 2023-03-29 11:26:52 +00:00 Compare
dstepanov-yadro force-pushed bug/OBJECT-2279-v2 from 23845a2fcb to 8e8d353743 2023-03-29 11:27:55 +00:00 Compare
fyrchik approved these changes 2023-03-29 11:56:10 +00:00
@ -0,0 +47,4 @@
return err
}
for _, o := range append(expiredNeoFS, expiredSys...) {
Owner

Do we need an append just for iterating?

Do we need an `append` just for iterating?
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
dstepanov-yadro force-pushed bug/OBJECT-2279-v2 from 8e8d353743 to 1077adfb13 2023-03-29 11:58:29 +00:00 Compare
fyrchik reviewed 2023-03-29 13:48:20 +00:00
@ -540,6 +545,7 @@ func (s *Shard) HandleExpiredLocks(lockers []oid.Address) {
var pInhume meta.InhumePrm
pInhume.SetAddresses(lockers...)
pInhume.SetGCMark()
Owner

We can (should) replace SetGCMark with SetForceGCMark, the former does nothing here.

We can (should) _replace_ `SetGCMark` with `SetForceGCMark`, the former does nothing here.
Author
Member

fixed

fixed
Contributor

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).

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).
Author
Member
// prevent lock objects to be inhumed
// if `Inhume` was called not with the
// `WithForceGCMark` option
if !prm.forceRemoval {
	if isLockObject(tx, cnr, id) {
		return ErrLockObjectRemoval
	}
	lockWasChecked = true
}
``` // prevent lock objects to be inhumed // if `Inhume` was called not with the // `WithForceGCMark` option if !prm.forceRemoval { if isLockObject(tx, cnr, id) { return ErrLockObjectRemoval } lockWasChecked = true } ```
Author
Member

I don't think we need a lot of different bool parameters with same function.

I don't think we need a lot of different ```bool``` parameters with same function.
Contributor

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

I don't think we need a lot of different bool parameters with same function.

it used to have a little bit different meaning before and currently it is useless, yes

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 > I don't think we need a lot of different bool parameters with same function. it used to have a little bit different meaning before and currently it is useless, yes
fyrchik approved these changes 2023-03-29 13:48:31 +00:00
dstepanov-yadro force-pushed bug/OBJECT-2279-v2 from 1077adfb13 to 17bc4a5216 2023-03-29 13:56:06 +00:00 Compare
dstepanov-yadro requested review from carpawell 2023-03-30 08:06:13 +00:00
carpawell approved these changes 2023-03-30 12:19:37 +00:00
carpawell left a comment
Contributor

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.

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.
Contributor

took me some time to understand what it does: i expect it to be named smth like Filter*. we have some Selects already in the code and all they do SELECT (mean going through all the objects they have and returning some subset)

not critical, just FYI

took me some time to understand what it does: i expect it to be named smth like `Filter*`. we have some `Select`s already in the code and all they do `SELECT` (mean going through all the objects they have and returning some subset) not critical, just FYI
Author
Member

fixed

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) {
Contributor

s/adresses/addresses?

s/adresses/addresses?
Author
Member

fixed

fixed
Author
Member

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.

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.

> 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. 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.
dstepanov-yadro force-pushed bug/OBJECT-2279-v2 from 17bc4a5216 to ab32067152 2023-03-30 12:34:04 +00:00 Compare
fyrchik merged commit ab32067152 into master 2023-03-30 12:38:37 +00:00
Contributor

Collection of expired locks and expired objects work concurrently both on the same shard and on different shards.

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.

> Collection of expired locks and expired objects work concurrently both on the same shard and on different shards. 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.
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-core-developers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#183
No description provided.