node: Improve shutdown #930

Merged
fyrchik merged 2 commits from acid-ant/frostfs-node:bugfix/improve-shutdown into master 2024-01-31 08:30:35 +00:00
Member

Stop internal activity of the gc by context.
Release task pool of the policer when context cancelled.

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

Stop internal activity of the `gc` by context. Release task pool of the `policer` when context cancelled. Signed-off-by: Anton Nikiforov <an.nikiforov@yadro.com>
acid-ant force-pushed bugfix/improve-shutdown from 4ecf408e19 to 748b173fc3 2024-01-25 06:32:42 +00:00 Compare
fyrchik requested review from storage-core-committers 2024-01-25 07:25:30 +00:00
fyrchik requested review from storage-core-developers 2024-01-25 07:25:30 +00:00
fyrchik reviewed 2024-01-25 07:41:57 +00:00
@ -339,1 +341,4 @@
return
default:
}
sh.NotificationChannel() <- ev
Owner

Shouldn't it be select with both this and <-ctx.Done() branches?
A separate select for ctx.Done() is ok, because this way we guarantee early exit, though a single select is not much worse.

Shouldn't it be `select` with both this and `<-ctx.Done()` branches? A separate `select` for `ctx.Done()` is ok, because this way we guarantee early exit, though a single select is not much worse.
Author
Member

Right, the idea was to exit earlier. And it looks like once NotificationChannel is buffered, we can use one select for this too statements.

Right, the idea was to exit earlier. And it looks like once `NotificationChannel` is buffered, we can use one `select` for this too statements.
@ -151,0 +150,4 @@
// Currently it is used only for listening for the new epoch event.
// It is ok to keep in buffer one event, it helps us properly react on cancelling of the context.
// In other case we need to keep the order of component shutdown, like closing morph before engine.
eventChan: make(chan Event, 1),
Owner

Is it necessary if we implement select suggestion from another comment?

Is it necessary if we implement `select` suggestion from another comment?
Author
Member

Yes, because there might be situation when notification received and at the same time context cancelled. In scope of this PR, routine which reading from channel might end earlier than routine which write in it - this will lead to block on write in channel.

Yes, because there might be situation when notification received and at the same time context cancelled. In scope of this PR, routine which reading from channel might end earlier than routine which write in it - this will lead to block on write in channel.
Owner

Hm, how exactly? If we have select with 2 branches and 1 is available (ctx.Done()), there is no way we will block

Hm, how exactly? If we have select with 2 branches and 1 is available (`ctx.Done()`), there is no way we will block
Author
Member

No more working from home, messed in your comments. Right, once select used for writing in eventChannel buffering here is redundant.

No more working from home, messed in your comments. Right, once `select` used for writing in `eventChannel` buffering here is redundant.

Does SIGHUP work correctly?

Does `SIGHUP` work correctly?
acid-ant force-pushed bugfix/improve-shutdown from 748b173fc3 to f01961bdf9 2024-01-25 08:03:30 +00:00 Compare
Author
Member

Does SIGHUP work correctly?

There are no changes in implementation for reaction on SIGHUP - for storage engine we are reconfiguring only shards.

> Does `SIGHUP` work correctly? There are no changes in implementation for reaction on SIGHUP - for `storage engine` we are reconfiguring only shards.
dstepanov-yadro approved these changes 2024-01-26 09:50:48 +00:00
acid-ant force-pushed bugfix/improve-shutdown from f01961bdf9 to ba7fa7eca1 2024-01-26 10:01:30 +00:00 Compare
acid-ant force-pushed bugfix/improve-shutdown from ba7fa7eca1 to 766ee97e81 2024-01-26 10:54:53 +00:00 Compare
acid-ant force-pushed bugfix/improve-shutdown from 766ee97e81 to 5f88bf9c77 2024-01-29 08:58:43 +00:00 Compare
acid-ant force-pushed bugfix/improve-shutdown from 5f88bf9c77 to 1f2c34318f 2024-01-30 07:14:12 +00:00 Compare
fyrchik approved these changes 2024-01-31 08:30:28 +00:00
fyrchik merged commit e3573de6db into master 2024-01-31 08:30:35 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-core-developers
No milestone
No project
No assignees
3 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#930
No description provided.