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

Merged
fyrchik merged 1 commits 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) {

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

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

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

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

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.

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?
Poster
Collaborator

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)

Why not nil?

Why not `nil`?
Poster
Collaborator

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{

An idea for a linter, actually.

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 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`?

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

Do we need an append just for iterating?

Do we need an `append` just for iterating?
Poster
Collaborator

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

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

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

fixed

fixed
Collaborator

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).
Poster
Collaborator
// 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 } ```
Poster
Collaborator

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

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
Collaborator

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

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
Poster
Collaborator

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

s/adresses/addresses?

s/adresses/addresses?
Poster
Collaborator

fixed

fixed
Poster
Collaborator

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
Collaborator

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 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
There is no content yet.