ir: Change log level on SIGHUP #258

Merged
fyrchik merged 1 commit from acid-ant/frostfs-node:feature/125-ir-sighup-log into master 2023-07-26 21:07:57 +00:00
Member

Related to #125

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

Related to #125 Signed-off-by: Anton Nikiforov <an.nikiforov@yadro.com>
acid-ant requested review from storage-core-committers 2023-04-17 06:26:09 +00:00
acid-ant requested review from storage-core-developers 2023-04-17 06:26:09 +00:00
aarifullin approved these changes 2023-04-17 06:37:49 +00:00
Owner

How is it related to #125?

How is it related to #125?
Author
Member

How is it related to #125?

We already have functionality to reload config but all of it located in cmd-node. This is the first part of refactoring - to set new log level on SIGHUP we need to reload config. At next steps of refactoring node I'm planning to move closers, workers and httpcomponent from node to internal/common/config. This will help to reload pprof and prometheus

> How is it related to #125? We already have functionality to reload config but all of it located in `cmd-node`. This is the first part of refactoring - to set new log level on SIGHUP we need to reload config. At next steps of refactoring node I'm planning to move `closers`, `workers` and `httpcomponent` from `node` to `internal/common/config`. This will help to reload `pprof` and `prometheus`
Owner

I would like to choose a lighter approach in IR, all this package-per-config-section packages in node are really heavy and an overkill for IR.

I would like to choose a lighter approach in IR, all this package-per-config-section packages in node are really heavy and an overkill for IR.
Author
Member

I would like to choose a lighter approach in IR, all this package-per-config-section packages in node are really heavy and an overkill for IR.

No, suggested to use only functions which related to config reload. Agree that package-per-config-section is a nightmare for IR and planning to use direct viper. For other components (httpcomponent, workers, closers) I think it is ok to reuse code from node.
Also found that currently in IR we don't wait for closing of pprof and prometheus.

> I would like to choose a lighter approach in IR, all this package-per-config-section packages in node are really heavy and an overkill for IR. No, suggested to use only functions which related to config reload. Agree that package-per-config-section is a nightmare for IR and planning to use direct `viper`. For other components (`httpcomponent`, `workers`, `closers`) I think it is ok to reuse code from node. Also found that currently in IR we don't wait for closing of pprof and prometheus.
acid-ant changed title from node: Move config from node to internal/common to WIP: node: Move config from node to internal/common 2023-04-17 10:26:52 +00:00
Author
Member

After discussion with @fyrchik had a decision to refactor cmd-node only in part of config reloading.

After discussion with @fyrchik had a decision to refactor `cmd-node` only in part of config reloading.
acid-ant force-pushed feature/125-ir-sighup-log from 91230f36d4 to 5193769165 2023-04-21 09:09:54 +00:00 Compare
acid-ant force-pushed feature/125-ir-sighup-log from 5193769165 to 57be7afbf0 2023-04-21 12:22:04 +00:00 Compare
acid-ant changed title from WIP: node: Move config from node to internal/common to ir: Change log level on SIGHUP 2023-04-21 12:25:36 +00:00
acid-ant requested review from aarifullin 2023-04-21 12:25:38 +00:00
acid-ant requested review from storage-core-developers 2023-04-21 12:25:44 +00:00
acid-ant force-pushed feature/125-ir-sighup-log from 57be7afbf0 to 3efc1e32e1 2023-04-21 12:26:28 +00:00 Compare
dstepanov-yadro reviewed 2023-04-21 15:41:49 +00:00
@ -48,3 +37,1 @@
if o.path != "" {
v.SetConfigFile(o.path)
func New(_ Prm, opts ...configViper.Option) *Config {

Remove _ Prm from parameters?

Remove ```_ Prm``` from parameters?
Author
Member

Removed it totally.

Removed it totally.
fyrchik marked this conversation as resolved
dstepanov-yadro reviewed 2023-04-21 15:44:02 +00:00
@ -0,0 +48,4 @@
for {
select {
case <-ctx.Done():

hmm. if ctx cancelled to it will be infinite loop.

hmm. if ctx cancelled to it will be infinite loop.
Author
Member

Thanks, removed this, refactored a bit main and config.

Thanks, removed this, refactored a bit main and config.
acid-ant force-pushed feature/125-ir-sighup-log from 3efc1e32e1 to 0760a7a6b2 2023-04-24 06:51:22 +00:00 Compare
acid-ant force-pushed feature/125-ir-sighup-log from 0760a7a6b2 to 145c2b2744 2023-04-24 07:15:51 +00:00 Compare
dstepanov-yadro approved these changes 2023-04-24 07:31:38 +00:00
fyrchik reviewed 2023-04-24 15:00:07 +00:00
@ -0,0 +48,4 @@
for {
select {
case err := <-intErr:
{
Owner

What value does this extra block have?

What value does this extra block have?
Author
Member

Removed redundant brackets.

Removed redundant brackets.
fyrchik marked this conversation as resolved
@ -144,2 +136,3 @@
}
return httpServers
//nolint:contextcheck
Owner

Let's avoid nolint in new code.

Let's avoid `nolint` in new code.
Author
Member

No more necessary to use //nolint:contextcheck here.

No more necessary to use `//nolint:contextcheck` here.
fyrchik marked this conversation as resolved
@ -35,7 +36,9 @@ func TestConfigEnv(t *testing.T) {
value = "some value"
)
err := os.Setenv(internal.Env(section, name), value)
Owner

Why the commit tells us it was unused?
Also, could use node: prefix in the commit message.

Why the commit tells us it was `unused`? Also, could use `node:` prefix in the commit message.
Author
Member

Fixed.

Fixed.
fyrchik marked this conversation as resolved
@ -0,0 +3,4 @@
import "github.com/spf13/viper"
type Opts struct {
Path string
Owner

Why are the fields public? In this case there is less need in functional options pattern (With*).

Why are the fields public? In this case there is less need in functional options pattern (`With*`).
Author
Member

Agree, refactored and changed to private, please review.

Agree, refactored and changed to private, please review.
fyrchik marked this conversation as resolved
acid-ant force-pushed feature/125-ir-sighup-log from 145c2b2744 to a2467f808e 2023-04-25 07:27:29 +00:00 Compare
acid-ant force-pushed feature/125-ir-sighup-log from a2467f808e to 54ce01225e 2023-04-25 07:51:32 +00:00 Compare
acid-ant force-pushed feature/125-ir-sighup-log from 54ce01225e to 25d67cff56 2023-04-25 10:10:55 +00:00 Compare
fyrchik approved these changes 2023-04-26 10:21:44 +00:00
acid-ant force-pushed feature/125-ir-sighup-log from 25d67cff56 to 14f83b8aa9 2023-04-26 10:55:46 +00:00 Compare
fyrchik merged commit 14f83b8aa9 into master 2023-04-26 10:56:39 +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#258
No description provided.