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

Merged
fyrchik merged 1 commit from acid-ant/frostfs-node:bugfix/364-fix-flush into master 2023-07-26 21:07:58 +00:00
Member

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
@ -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 >
Owner

What a beautiful separate commit!

What a beautiful separate commit!
fyrchik marked this conversation as resolved
fyrchik reviewed 2023-05-22 08:13:24 +00:00
@ -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
Owner

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.
Author
Member

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.
fyrchik marked this conversation as resolved
Owner

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?
Author
Member

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

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

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

@acid-ant The @dstepanov-yadro suggestion looks fine to me.
acid-ant force-pushed bugfix/364-fix-flush from 75c9e88972 to 2575f6c00d 2023-05-24 11:12:02 +00:00 Compare
Author
Member

@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
@ -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) {
Owner

Shouldn't it respect context too?

Shouldn't it respect context too?
Author
Member

I prefer to do this in separate PR - another bunch of files need to refactor. Created #394 for tracking.

I prefer to do this in separate PR - another bunch of files need to refactor. Created #394 for tracking.
acid-ant force-pushed bugfix/364-fix-flush from 2575f6c00d to 41cb98110b 2023-05-25 13:49:42 +00:00 Compare
dstepanov-yadro approved these changes 2023-05-25 15:00:18 +00:00
acid-ant force-pushed bugfix/364-fix-flush from 41cb98110b to 86a52e4de9 2023-05-26 12:15:22 +00:00 Compare
acid-ant force-pushed bugfix/364-fix-flush from 86a52e4de9 to 802168c0c6 2023-05-26 13:47:08 +00:00 Compare
aarifullin approved these changes 2023-05-26 13:50:10 +00:00
fyrchik approved these changes 2023-05-26 13:52:33 +00:00
fyrchik merged commit 802168c0c6 into master 2023-05-26 13:52:53 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
4 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#379
No description provided.