Simple object with lock and expiration time not removed after locks are expired #145

dstepanov-yadro · 2023-03-17T08:37:51Z

dstepanov-yadro commented

2023-03-17 08:37:51 +00:00

Let say we have:

Current epoch is 100
Simple object with NEOFSEXPIRATIONEPOCH=101
Simple object is locked with lock until epoch 103

Steps:
Tick epochs until current epoch = 103
Check object availability

Expected Behavior
Simple object should be removed since locks are gone and object expired

Current Behavior
Simple object continues to present in storage even if we wait additional extra epochs

**Let say we have:** Current epoch is 100 Simple object with NEOFSEXPIRATIONEPOCH=101 Simple object is locked with lock until epoch 103 **Steps:** Tick epochs until current epoch = 103 Check object availability **Expected Behavior** Simple object should be removed since locks are gone and object expired **Current Behavior** Simple object continues to present in storage even if we wait additional extra epochs

dstepanov-yadro force-pushed bug/OBJECT-2279 from 705aad7370 to 699485924e

2023-03-17 08:40:48 +00:00

Compare

dstepanov-yadro changed title from ~~WIP: Simple object with lock and expiration time not removed after locks are expired~~ to Simple object with lock and expiration time not removed after locks are expired

2023-03-17 08:41:10 +00:00

dstepanov-yadro requested review from storage-core-committers 2023-03-17 08:41:21 +00:00

dstepanov-yadro requested review from storage-core-developers 2023-03-17 08:41:21 +00:00

fyrchik reviewed 2023-03-20 13:15:44 +00:00

cmd/frostfs-node/config.go Outdated

					
				@ -121,1 +120,3 @@

						removerSleepInterval time.Duration

						removerBatchSize             int

						removerSleepInterval         time.Duration

						expiredCollectorBatchSize    int

fyrchik commented

2023-03-20 13:11:04 +00:00

With many params our system will be harder to configure (the process of fine-tuning and determining what the best value is is too slow and error-prone). We already have many complex dependencies between parameter values and system behaviour.

Can we just take the same batch size? Do we have a reason, why should they be different?

With many params our system will be harder to configure (the process of fine-tuning and determining what _the best_ value is is too slow and error-prone). We already have many complex dependencies between parameter values and system behaviour. Can we just take the same batch size? Do we have a reason, why should they be different?

dstepanov-yadro commented

2023-03-20 15:26:29 +00:00

removerBatchSize is batch size for removing objects.

expiredCollectorBatchSize is batch size for marking objects expired.

These parameters relate to different processes. In the case of one parameter, we will create another complex dependency.

```removerBatchSize``` is batch size for removing objects. ```expiredCollectorBatchSize``` is batch size for marking objects expired. These parameters relate to different processes. In the case of one parameter, we will create another complex dependency.

fyrchik marked this conversation as resolved

cmd/frostfs-node/config.go Outdated

					
				@ -122,0 +120,4 @@

						removerBatchSize             int

						removerSleepInterval         time.Duration

						expiredCollectorBatchSize    int

						expiredCollectorWorkersCount int

fyrchik commented

2023-03-20 13:15:39 +00:00

Don't forget to update docs/storage-node-configuration.md.

Don't forget to update `docs/storage-node-configuration.md`.

dstepanov-yadro commented

2023-03-21 06:52:04 +00:00

Done.

fyrchik marked this conversation as resolved

pkg/local_object_storage/shard/gc.go Outdated

					
				@ -242,0 +268,4 @@

					errGroup, egCtx := errgroup.WithContext(ctx)

					errGroup.SetLimit(workersCount)

					errGroup.Go(func() error {

fyrchik commented

2023-03-20 13:13:49 +00:00

Why do we need a goroutine here?

dstepanov-yadro commented

2023-03-20 15:13:22 +00:00

If s.getExpiredObjects fails with an error then all other goroutines executing s.handleExpiredObjects will be cancelled by egCtx, and vice versa. It is possible to write code without this goroutine, but code will be much more tricky.

If ```s.getExpiredObjects``` fails with an error then all other goroutines executing ```s.handleExpiredObjects``` will be cancelled by ```egCtx```, and vice versa. It is possible to write code without this goroutine, but code will be much more tricky.

👍 1

fyrchik marked this conversation as resolved

dstepanov-yadro force-pushed bug/OBJECT-2279 from 699485924e to acf79b8425

2023-03-21 06:44:22 +00:00

Compare

fyrchik approved these changes 2023-03-21 07:55:33 +00:00

pkg/local_object_storage/shard/gc.go Outdated

					
				@ -237,0 +248,4 @@

					s.collectExpiredObjects(ctx, e)

				}

				func (s *Shard) getExpiredObjectsParameters() (workersCount, batchSize int) {

fyrchik commented

2023-03-21 07:43:47 +00:00

Why not doing it once on init?

dstepanov-yadro commented

2023-03-21 08:24:25 +00:00

source code looks simpler when variable initialization is near variable usage. getExpiredObjectsParameters() doesn't look hard to run.
it will be easier to support sighup for gc parameters.

1. source code looks simpler when variable initialization is near variable usage. ```getExpiredObjectsParameters()``` doesn't look hard to run. 2. it will be easier to support sighup for gc parameters.

fyrchik commented

2023-03-21 08:52:55 +00:00

I somewhat agree, but here we add a function which looks unnecessary batchSize := c.gcCfg.getExpiredObjectBatchSize doesn't make the code simpler to me.

Also, how would SIGHUP become easier?

I somewhat agree, but here we add a function which looks unnecessary `batchSize := c.gcCfg.getExpiredObjectBatchSize` doesn't make the code simpler to me. Also, how would SIGHUP become easier?

fyrchik commented

2023-03-21 08:55:42 +00:00

I guess I see, we could lock in the function and lock during SIGHUP.