fix/Nil panic in control service #2655
Conversation
carpawell
requested review from
roman-khimov and
cthulhu-rider
as code owners
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2655 +/- ##
==========================================
- Coverage 30.62% 30.61% -0.01%
==========================================
Files 406 406
Lines 30070 30066 -4
==========================================
- Hits 9208 9206 -2
+ Misses 20085 20082 -3
- Partials 777 778 +1 ☔ View full report in Codecov by Sentry. |
|
|
||
| func (s *Server) ready() error { | ||
| if !s.available.Load() { | ||
| return status.Error(codes.Unavailable, "node has not started fully yet") |
There was a problem hiding this comment.
to me node is smth higher level. I'd report service has not been completely initialized yet
| @@ -13,6 +14,12 @@ import ( | |||
| // Server is an entity that serves | |||
| // Control service on storage node. | |||
| type Server struct { | |||
| // initialization sync; control service | |||
| // is the first service to run so no other | |||
There was a problem hiding this comment.
first service to run is strange to see on the individual service: this field doesnt "know" what the app does, the app knows how Service behaves
There was a problem hiding this comment.
i can understand you but think that is too much of an abstraction then, do not think the control service can be reused anywhere else, only for the node and only as a main controlling service
this field doesnt "know" what the app does
yes, but we need this service to be split now. not because it is good for it but because we need it for its caller and this comment would allow a future reader to understand it faster
anyway, fixed the comment
cmd/neofs-node/control.go
Outdated
| @@ -70,6 +58,17 @@ func initControlService(c *cfg) { | |||
| c.wg.Done() | |||
| }) | |||
| }() | |||
|
|
|||
| c.workers = append(c.workers, newWorkerFromFunc(func(ctx context.Context) { | |||
There was a problem hiding this comment.
why async worker? U know exactly when all needed components are ready
There was a problem hiding this comment.
think that is our common pattern but it was about IR, fixed
| } | ||
| // MarkReady marks server available. Before this call none of the other calls | ||
| // are available except for the health checks. | ||
| func (s *Server) MarkReady(e *engine.StorageEngine, nm netmap.Source, c container.Source, r *replicator.Replicator, st NodeState, tr TreeService) { |
There was a problem hiding this comment.
suggest to require all prms to be non-nil and insta panic otherwise
Split service creation on two steps: 1. service creation with key initialization and basic health checks; 2. service real initialization that includes internals reading (such as storage engine, tree service, etc). The first one handles incoming requests when the app starts, the second one is responsible for the full work mode (storage engine initializations may take time in the real runs with the real objects stored). Signed-off-by: Pavel Karpy <[email protected]>
carpawell
force-pushed
the
fix/control-svc-panic
branch
from
2825376 to
77404cc
Compare
Health checks (the can be improved, see #2585 (comment)) and closing #2585. |
|
OK, we can have some more in the future. |
Split service creation on two steps:
The first one handles incoming requests when the app starts, the second one is responsible for the full work mode (storage engine initializations may take time in the real runs with the real objects stored).