Use LockPrm and LockRes to store Lock operation parameters and results #1556

a-savchuk · 2024-12-11T13:50:08Z

a-savchuk commented

2024-12-11 13:50:08 +00:00

Refactor Lock operation to use LockPrm and LockRes structures to store operation parameters and results, respectively

Close #1421 Refactor `Lock` operation to use `LockPrm` and `LockRes` structures to store operation parameters and results, respectively

requested reviews from storage-core-committers, storage-core-developers

2024-12-11 13:50:09 +00:00

dstepanov-yadro reviewed 2024-12-11 15:04:57 +00:00

pkg/local_object_storage/metabase/lock.go Outdated

					
				@ -33,0 +45,4 @@

				// SetTarget sets lock object ID and IDs of objects to be locked.

				//

				// The list of locked objects should be unique. Panics if the list is empty.

dstepanov-yadro commented

2024-12-11 15:04:57 +00:00

But there is no check if list is empty and no panic...

dstepanov-yadro commented

2024-12-11 15:07:42 +00:00

Three arguments is not so much. Moreover, in all functions, the first step is to expand the structure:

func (db *DB) Lock(ctx context.Context, prm LockPrm) (LockRes, error) {
	cnr := prm.cnt
	locker := prm.lock
	locked := prm.locked
	
...

Three arguments is not so much. Moreover, in all functions, the first step is to expand the structure: ``` func (db *DB) Lock(ctx context.Context, prm LockPrm) (LockRes, error) { cnr := prm.cnt locker := prm.lock locked := prm.locked ... ```

👍 1

a-savchuk force-pushed add-lock-prm from 6f28a52e1a to fb5c4b0ad3

2024-12-11 22:39:59 +00:00

Compare

fyrchik commented

2024-12-12 03:38:16 +00:00

@a-savchuk what is the motivation for this PR?

a-savchuk commented

2024-12-12 08:13:40 +00:00

The initial motivation was to have a flag with a default value for the Lock and Inhume operations while working on #1445. The flag was meant to be used for writing tests those need to populate a metabase with locks of both old and new formats. Consider the following code snippet:

type LockPrm struct {
    // ...
    expEpoch uint64
   
   // Flag default value is false.
   // Should be used for test purposes only.
    skipExpEpochCheck bool
}

// ...

func TestLock(t *testing.T) {
    db := newDB(t)
    
    var prm meta.LockPrm
    
    prm.SetExpEpoch(100)
    _, _ = db.Lock(ctx, prm)
    
    prm.SetExpEpoch(meta.NoExpEpoch)
    prm.SkipExpEpochCheck()
    _, _ = db.Lock(ctx, prm)
}

Discussed with @fyrchik IRL
Decided not to add expiration epoch check at all and make setting expiration epoch explicit with lock target

The initial motivation was to have a flag with a default value for the Lock and Inhume operations while working on https://git.frostfs.info/TrueCloudLab/frostfs-node/issues/1445. The flag was meant to be used for writing tests those need to populate a metabase with locks of both old and new formats. Consider the following code snippet: ```go type LockPrm struct { // ... expEpoch uint64 // Flag default value is false. // Should be used for test purposes only. skipExpEpochCheck bool } // ... func TestLock(t *testing.T) { db := newDB(t) var prm meta.LockPrm prm.SetExpEpoch(100) _, _ = db.Lock(ctx, prm) prm.SetExpEpoch(meta.NoExpEpoch) prm.SkipExpEpochCheck() _, _ = db.Lock(ctx, prm) } ``` Discussed with @fyrchik IRL Decided not to add expiration epoch check at all and make setting expiration epoch explicit with lock target

a-savchuk closed this pull request

2024-12-12 08:13:41 +00:00

a-savchuk deleted branch add-lock-prm

2024-12-12 08:13:51 +00:00