[#653] Pass context.Context to StorageEngine Open #672

elebedeva · 2023-08-31T16:44:52Z

elebedeva commented

2023-08-31 16:44:52 +00:00

Close #653

Signed-off-by: Ekaterina Lebedeva ekaterina.lebedeva@yadro.com

Close #653 Signed-off-by: Ekaterina Lebedeva <ekaterina.lebedeva@yadro.com>

fyrchik reviewed 2023-08-31 17:00:24 +00:00

fyrchik left a comment

It seems you have a really old branch, please rebase on the actual master.

cmd/frostfs-lens/internal/meta/root.go Outdated

					
				@ -44,3 +45,3 @@

						meta.WithEpochState(epochState{}),

					)

					common.ExitOnErr(cmd, common.Errf("could not open metabase: %w", db.Open(true)))

					common.ExitOnErr(cmd, common.Errf("could not open metabase: %w", db.Open(context.Background(), true)))

fyrchik commented

2023-08-31 16:51:56 +00:00

cmd.Context() is what we usually use for this.

`cmd.Context()` is what we usually use for this.

elebedeva commented

2023-09-01 12:39:25 +00:00

Fixed.

fyrchik marked this conversation as resolved

fyrchik commented

2023-09-01 08:57:00 +00:00

Don't forget to add reviewers! Our team has core- in it's name -- pick both options.

Don't forget to add reviewers! Our team has `core-` in it's name -- pick both options.

👍 1

elebedeva force-pushed open-context from a454b19589 to b60014d415

2023-09-01 12:36:59 +00:00

Compare

elebedeva requested review from storage-core-developers 2023-09-01 12:39:04 +00:00

elebedeva requested review from storage-core-committers 2023-09-01 12:39:05 +00:00

dstepanov-yadro requested changes 2023-09-01 12:55:12 +00:00

cmd/frostfs-node/config.go Outdated

					
				@ -932,3 +932,3 @@

						c.log.Info(logs.FrostFSNodeClosingComponentsOfTheStorageEngine)

						err := ls.Close()

						err := ls.Close(ctx)

dstepanov-yadro commented

2023-09-01 12:49:41 +00:00

Need to check: if onShutdown calls afetr ctx is cancelled, then Close() may fail. So context.Background() or detached https://pkg.go.dev/context#WithoutCancel should be used.

Need to check: if `onShutdown` calls afetr ctx is cancelled, then `Close()` may fail. So `context.Background()` or detached https://pkg.go.dev/context#WithoutCancel should be used.

fyrchik commented

2023-09-04 07:58:57 +00:00

WithoutCancel() was added in 1.21, but we need to support 1.20 too.

`WithoutCancel()` was added in 1.21, but we need to support 1.20 too.

elebedeva commented

2023-09-05 10:25:37 +00:00

changed ls.Close(ctx) to ls.Close(context.Background())

changed `ls.Close(ctx)` to `ls.Close(context.Background())`

fyrchik marked this conversation as resolved

pkg/local_object_storage/blobstor/mode.go Outdated

					
				@ -22,3 +23,3 @@

					err := b.Close()

					if err == nil {

						if err = b.Open(m.ReadOnly()); err == nil {

						if err = b.Open(context.Background(), m.ReadOnly()); err == nil {

dstepanov-yadro commented

2023-09-01 12:51:18 +00:00

context.TODO is more preferred in such cases (or we should pass ctx to SetMode too :) )

elebedeva commented

2023-09-05 10:24:26 +00:00

fixed

fyrchik marked this conversation as resolved

pkg/local_object_storage/metabase/mode.go Outdated

					
				@ -28,3 +29,3 @@

						db.boltDB = nil

					case m.ReadOnly():

						err = db.Open(true)

						err = db.Open(context.Background(), true)

dstepanov-yadro commented

2023-09-01 12:53:57 +00:00

context.TODO too

elebedeva commented

2023-09-05 10:24:15 +00:00

fixed

fyrchik marked this conversation as resolved

elebedeva force-pushed open-context from b60014d415 to ce614e8a94

2023-09-05 10:23:16 +00:00

Compare

fyrchik reviewed 2023-09-05 11:06:38 +00:00

pkg/local_object_storage/engine/engine.go Outdated

					
				@ -177,3 +179,3 @@

					isMeta := errors.As(err, new(metaerr.Error))

					if block {

						e.moveToDegraded(sh, errCount, isMeta)

						e.moveToDegraded(ctx, sh, errCount, isMeta)

fyrchik commented

2023-09-05 11:06:25 +00:00

Same here the ctx here is taken from operation, I believe SetMode should not be affected by it.

Same here the `ctx` here is taken from operation, I believe `SetMode` should not be affected by it.

fyrchik marked this conversation as resolved

pkg/local_object_storage/shard/control.go Outdated

					
				@ -57,2 +59,3 @@

					for i, component := range components {

						if err := component.Open(false); err != nil {

						if err := component.Open(ctx, false); err != nil {

							select {

fyrchik commented

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

What is this select for? We have an error, no need to check the context. For the component == s.metaBase check it is already checked in the loop.

What is this select for? We have an error, no need to check the context. For the `component == s.metaBase` check it is already checked in the loop.

👍 1

elebedeva commented

2023-09-07 15:13:39 +00:00

Removed unnecessary select

fyrchik marked this conversation as resolved

pkg/services/control/server/set_shard_mode.go Outdated

					
				@ -38,3 +38,3 @@

					for _, shardID := range s.getShardIDList(req.Body.GetShard_ID()) {

						err = s.s.SetShardMode(shardID, m, req.Body.GetResetErrorCounter())

						err = s.s.SetShardMode(ctx, shardID, m, req.Body.GetResetErrorCounter())

fyrchik commented

2023-09-05 11:00:09 +00:00

This may be a problem -- ctx is a context for stream operation, while ctx for Open/Init/SetMode is for the storage lifetime. Have you checked how this context is used?

This may be a problem -- ctx is a context for stream operation, while ctx for `Open`/`Init`/`SetMode` is for the storage lifetime. Have you checked how this context is used?

fyrchik marked this conversation as resolved

elebedeva force-pushed open-context from ce614e8a94 to 3327355ebc

2023-09-07 14:53:18 +00:00

Compare

elebedeva force-pushed open-context from 3327355ebc to 8a81af5a3b

2023-09-07 15:08:13 +00:00

Compare

fyrchik approved these changes 2023-09-07 15:10:28 +00:00

aarifullin approved these changes 2023-09-07 16:08:24 +00:00

aarifullin left a comment

Very nice!

fyrchik merged commit 8a81af5a3b into master