frostfs-node refactorings #168

dstepanov-yadro · 2023-03-23T15:03:12Z

dstepanov-yadro commented

2023-03-23 15:03:12 +00:00

No description provided.

dstepanov-yadro force-pushed refactoring/OBJECT-3610_node from 4254f4efca to e50d119107

2023-03-23 15:37:58 +00:00

Compare

dstepanov-yadro force-pushed refactoring/OBJECT-3610_node from 1da212f3e9 to 4f619c99a5

2023-03-24 12:10:03 +00:00

Compare

dstepanov-yadro changed title from ~~WIP: frostfs-node refactorings~~ to frostfs-node refactorings

2023-03-24 12:10:12 +00:00

fyrchik reviewed 2023-03-27 11:10:24 +00:00

cmd/frostfs-node/reputation.go Outdated

					
				@ -93,1 +117,4 @@

					)

				}

				func addEpochHandler(c *cfg, eigenTrustController *eigentrustctrl.Controller) {

fyrchik commented

2023-03-27 10:53:55 +00:00

The name seems to generic, given the function signature.

dstepanov-yadro commented

2023-03-27 16:09:53 +00:00

fixed

fyrchik marked this conversation as resolved

pkg/local_object_storage/engine/control.go Outdated

					
				@ -265,3 +266,3 @@

				// Reload reloads StorageEngine's configuration in runtime.

				func (e *StorageEngine) Reload(rcfg ReConfiguration) error {

				func (e *StorageEngine) Reload(ctx context.Context, rcfg ReConfiguration) error {

fyrchik commented

2023-03-27 10:58:50 +00:00

It is tricky from the code POV: nothing prevents us from passing different contexts on Init and Reload -- this will lead to different shards living in different worlds (possibly, some of the shards exiting early).

To me it looks, like storing the context somewhere in the engine is not so bad. cc @carpawell @ale64bit

It is tricky from the code POV: nothing prevents us from passing different contexts on `Init` and `Reload` -- this will lead to different shards living in different worlds (possibly, some of the shards exiting early). To me it looks, like storing the context somewhere in the `engine` is not so bad. cc @carpawell @ale64bit

ale64bit commented

2023-03-27 11:15:26 +00:00

IMHO, neither the context should be stored in a struct (context docs discourage this explicitly) nor the state/functioning of the engine instance should depend on something stored in the context in the first place.

IMHO, neither the context should be stored in a struct ([context](https://pkg.go.dev/context) docs discourage this explicitly) nor the state/functioning of the engine instance should depend on something stored in the context in the first place.

👍 1

fyrchik commented

2023-03-30 11:09:55 +00:00

Dunno, it bugs be that this ctx is not a context for the Reload, but actually a context for the created shard.
I would at least rename the variable or mention this in doc. But ok, if it seems good to you.

Dunno, it bugs be that this `ctx` is not a context for the `Reload`, but actually a context for the created shard. I would at least rename the variable or mention this in doc. But ok, if it seems good to you.

dstepanov-yadro force-pushed refactoring/OBJECT-3610_node from 4f619c99a5 to 018dd74f69

2023-03-27 16:09:40 +00:00

Compare

dstepanov-yadro requested review from storage-core-committers 2023-03-28 06:20:24 +00:00

dstepanov-yadro requested review from storage-core-developers 2023-03-28 06:20:25 +00:00

dstepanov-yadro force-pushed refactoring/OBJECT-3610_node from 018dd74f69 to 1c3a1cd39a

2023-03-29 14:40:14 +00:00

Compare

fyrchik approved these changes 2023-03-30 11:08:44 +00:00

carpawell approved these changes 2023-03-30 15:58:18 +00:00

cmd/frostfs-node/config.go Outdated

					
				@ -1034,3 +1096,3 @@

					c.setHealthStatus(control.HealthStatus_SHUTTING_DOWN)

					c.ctxCancel()

					c.done <- struct{}{}

carpawell commented

2023-03-30 15:41:44 +00:00

may a closing be a more common way? it is more a question than a suggestion

dstepanov-yadro force-pushed refactoring/OBJECT-3610_node from 1c3a1cd39a to ed28ce24cd

2023-03-31 06:33:21 +00:00

Compare

fyrchik approved these changes 2023-03-31 06:50:26 +00:00

fyrchik merged commit ed28ce24cd into master

2023-03-31 06:50:32 +00:00