Add storage.low_mem config parameter #461

dstepanov-yadro · 2023-06-22T07:55:50Z

dstepanov-yadro commented

2023-06-22 07:55:50 +00:00

Closes #428

Now this parameter prohibits parallel metabase resync only. In case of many shards and low memory available on machine it can lead to OOM.

I haven't specify this parameter in config example as it will be rarely used i think.

Closes #428 Now this parameter prohibits parallel metabase resync only. In case of many shards and low memory available on machine it can lead to OOM. I haven't specify this parameter in config example as it will be rarely used i think.

acid-ant approved these changes 2023-06-22 14:12:29 +00:00

fyrchik approved these changes 2023-06-23 08:10:06 +00:00

pkg/local_object_storage/engine/control.go Outdated

													
				@ -79,0 +80,4 @@

					if e.cfg.lowMem && e.anyShardRequiresRefill() {

						concurrentLimit = 1

					}

					sem := make(chan struct{}, concurrentLimit)

fyrchik commented

2023-06-23 08:07:31 +00:00

Why don't we just use errgroup?

Why don't we just use `errgroup`?

dstepanov-yadro commented

2023-06-23 11:16:44 +00:00

Poster

fixed

fyrchik marked this conversation as resolved

dstepanov-yadro force-pushed fix/lowmem_config from e83612b6bc to ae04be3fa9

2023-06-23 11:05:43 +00:00

Compare

dstepanov-yadro force-pushed fix/lowmem_config from ae04be3fa9 to 5bcf708766

2023-06-23 11:16:17 +00:00

Compare

acid-ant approved these changes 2023-06-23 12:09:49 +00:00

ale64bit reviewed 2023-06-23 12:14:18 +00:00

pkg/local_object_storage/engine/control.go Outdated

													
				@ -76,3 +77,3 @@

					var wg sync.WaitGroup

					var errCh = make(chan shardInitError, len(e.shards))

					eg, _ := errgroup.WithContext(ctx)

ale64bit commented

2023-06-23 12:14:18 +00:00

why ignore the derived context? shouldn't it be used by sh.Init(ctx) below?

why ignore the derived context? shouldn't it be used by `sh.Init(ctx)` below?

dstepanov-yadro commented

2023-06-23 13:13:48 +00:00

Poster

eg.Go doesn't return error, so there is no need to use derived ctx. Derived ctx needed in case of some goroutine returns error, then derived ctx will be cancelled.

`eg.Go` doesn't return error, so there is no need to use derived ctx. Derived ctx needed in case of some goroutine returns error, then derived ctx will be cancelled.

ale64bit commented

2023-06-23 16:45:03 +00:00

well, but that was my point, to stop further initialization if one of them failed already.

dstepanov-yadro commented

2023-06-26 06:53:28 +00:00

Poster

ok, fixed

fyrchik commented

2023-06-26 07:53:49 +00:00

Well, actually, no, this is a feature -- if ONE shard cannot be initialized, we just remove it and continue initialization for others. Adding context doesn't change it actually, because we sent an error to the channel.

However I am a bit concerned now:
From https://pkg.go.dev/golang.org/x/sync/errgroup#WithContext (cursive is mine):

The derived Context is canceled the first time a function passed to Go returns a non-nil error or the first time Wait returns

And this context is passed to GC, have you checked it doesn't lead to problems?

Well, actually, no, this is a feature -- if ONE shard cannot be initialized, we just remove it and continue initialization for others. Adding context doesn't change it actually, because we sent an error to the channel. However I am a bit concerned now: From https://pkg.go.dev/golang.org/x/sync/errgroup#WithContext (cursive is mine): > The derived Context is canceled the first time a function passed to Go returns a non-nil error or *the first time Wait returns* And this context is passed to GC, have you checked it doesn't lead to problems?

fyrchik commented

2023-06-26 07:55:38 +00:00

May be indeed #461 (comment) was a mistake.

May be indeed https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/461#issuecomment-13618 was a mistake.

ale64bit commented

2023-06-26 09:13:08 +00:00

if that's the case then don't use WithContext in the first place? i.e. simply var eg errgroup.Group. Otherwise it's kinda misleading.

if that's the case then don't use `WithContext` in the first place? i.e. simply `var eg errgroup.Group`. Otherwise it's kinda misleading.

👍 1

dstepanov-yadro commented

2023-06-26 09:27:19 +00:00

Poster