node: Stop flushing big object when termination signal received #379

acid-ant · 2023-05-19T09:44:43Z

acid-ant commented

2023-05-19 09:44:43 +00:00

Close #364

Signed-off-by: Anton Nikiforov an.nikiforov@yadro.com

Close #364 Signed-off-by: Anton Nikiforov <an.nikiforov@yadro.com>

acid-ant requested review from storage-core-committers 2023-05-19 09:44:54 +00:00

acid-ant requested review from storage-core-developers 2023-05-19 09:44:54 +00:00

acid-ant changed title from ~~node: Stop flushing big object when termination signal received~~ to WIP: node: Stop flushing big object when termination signal received

2023-05-19 10:42:17 +00:00

acid-ant force-pushed bugfix/364-fix-flush from 27f76ee280 to a525fe391b

2023-05-22 07:04:42 +00:00

Compare

acid-ant changed title from ~~WIP: node: Stop flushing big object when termination signal received~~ to node: Stop flushing big object when termination signal received

2023-05-22 07:07:30 +00:00

fyrchik approved these changes 2023-05-22 08:12:52 +00:00

docs/evacuation.md Outdated

													
				@ -86,3 +86,3 @@

				```bash

				frostfs-cli control shards evacuation start --endpoint s01.frostfs.devenv:8081 --wallet ./../frostfs-dev-env/services/storage/wallet01.json --id 54Y8aot9uc7BSadw2XtYr3 --await --no-progress

				Enter password > 

				Enter password >

fyrchik commented

2023-05-22 08:12:50 +00:00

What a beautiful separate commit!

❤️ 2

fyrchik marked this conversation as resolved

fyrchik reviewed 2023-05-22 08:13:24 +00:00

pkg/local_object_storage/writecache/flush.go Outdated

													
				@ -210,6 +210,7 @@ func (c *cache) flushFSTree(ctx context.Context, ignoreErrors bool) error {

						c.deleteFromDisk(ctx, []string{sAddr})

						return nil

					}

					prm.CloseCh = c.closeCh

fyrchik commented

2023-05-22 08:13:24 +00:00

Have you run tests with the -race flag? c.closeCh can be unsafe to access.

Have you run tests with the `-race` flag? `c.closeCh` can be unsafe to access.

acid-ant commented

2023-05-22 08:48:48 +00:00

Poster

Yes, and they have passed. We are setting closeCh as nil in Close() and after c.wg.Wait() completed.

Yes, and they have passed. We are setting closeCh as nil in `Close()` and after `c.wg.Wait()` completed.

👍 1

fyrchik marked this conversation as resolved

fyrchik commented

2023-05-22 08:37:46 +00:00

Also, about providing context like it is done for other methods in common.Storage interface?

Also, about providing context like it is done for other methods in `common.Storage` interface?

acid-ant commented

2023-05-22 09:05:10 +00:00

Poster

Also, about providing context like it is done for other methods in common.Storage interface?

I thought about this, that was first implementation. But it takes a lot of changes, not only in Init() for writecache - SetMode(), Open() also need to use context from main.
Also, we need to redesign main shutdown function - use Context from main everywhere, create child context for each component and close main only when all children already closed to keep shutdown process manageable.
We discussed this with @dstepanov-yadro, and it looks like Context is more about requests, not about how to control application lifecycle. @fyrchik what do you think?

> Also, about providing context like it is done for other methods in `common.Storage` interface? I thought about this, that was first implementation. But it takes a lot of changes, not only in `Init()` for `writecache` - `SetMode()`, `Open()` also need to use context from main. Also, we need to redesign main shutdown function - use Context from main everywhere, create child context for each component and close main only when all children already closed to keep shutdown process manageable. We discussed this with @dstepanov-yadro, and it looks like Context is more about requests, not about how to control application lifecycle. @fyrchik what do you think?

aarifullin approved these changes 2023-05-22 10:25:19 +00:00

acid-ant force-pushed bugfix/364-fix-flush from a525fe391b to 75c9e88972

2023-05-23 06:45:08 +00:00

Compare

dstepanov-yadro commented

2023-05-23 09:26:04 +00:00

As discussed with @acid-ant we can use such trick and pass ctx to flush sub-calls:

func (c *cache) runFlushLoop() {
	ctx, cancel := context.WithCancel(context.Background())
	defer cancel()

	go func() {
		<-c.closeCh
		cancel()
	}()

It is more golang-way i think.

As discussed with @acid-ant we can use such trick and pass ctx to flush sub-calls: ``` func (c *cache) runFlushLoop() { ctx, cancel := context.WithCancel(context.Background()) defer cancel() go func() { <-c.closeCh cancel() }() ``` It is more golang-way i think.

👍 1

fyrchik commented

2023-05-24 07:57:22 +00:00

@acid-ant The @dstepanov-yadro suggestion looks fine to me.

👍 1

acid-ant force-pushed bugfix/364-fix-flush from 75c9e88972 to 2575f6c00d

2023-05-24 11:12:02 +00:00

Compare

acid-ant commented

2023-05-24 11:13:36 +00:00

Poster

@acid-ant The @dstepanov-yadro suggestion looks fine to me.

Updated, @fyrchik and @dstepanov-yadro please review.

> @acid-ant The @dstepanov-yadro suggestion looks fine to me. Updated, @fyrchik and @dstepanov-yadro please review.

acid-ant requested review from fyrchik 2023-05-24 11:13:53 +00:00

acid-ant requested review from aarifullin 2023-05-24 11:13:56 +00:00

aarifullin approved these changes 2023-05-24 15:34:58 +00:00

acid-ant scheduled this pull request to auto merge when all checks succeed 2023-05-24 18:29:01 +00:00

fyrchik approved these changes 2023-05-25 10:59:26 +00:00

pkg/local_object_storage/blobstor/blobovniczatree/iterate.go Outdated

													
				@ -12,3 +13,3 @@

				// Iterate iterates over all objects in b.

				func (b *Blobovniczas) Iterate(prm common.IteratePrm) (common.IterateRes, error) {

				func (b *Blobovniczas) Iterate(_ context.Context, prm common.IteratePrm) (common.IterateRes, error) {