TrueCloudLab/frostfs-s3-gw
17
3
Fork 20
Code Issues 26 Pull requests 6 Projects Releases 24 Packages 1 Activity Actions

app: Reload copies numbers on SIGHUP #112

Merged
alexvanin merged 1 commit from ironbee/frostfs-s3-gw:update-copies-numbers-on-SIGHUP into master 2023-05-23 10:28:58 +00:00
Conversation 0 Commits 1 Files changed 7 +105 -50
ironbee commented 2023-05-18 13:39:32 +00:00
Contributor
Copy link

Signed-off-by: Artem Tataurov a.tataurov@yadro.com

Closes: #104

Signed-off-by: Artem Tataurov <a.tataurov@yadro.com> Closes: #104
ironbee requested review from storage-services-committers 2023-05-18 13:39:43 +00:00
ironbee requested review from storage-services-developers 2023-05-18 13:39:43 +00:00
alexvanin requested changes 2023-05-19 06:58:23 +00:00
cmd/s3-gw/app.go Outdated
@ -310,0 +338,4 @@
p.mu.RLock()
copiesNumbers := p.defaultCopiesNumbers
p.mu.RUnlock()
return copiesNumbers
alexvanin commented 2023-05-19 06:42:14 +00:00
Owner
Copy link

Cosmetic: function above adds empty line before return state and this isn't.

Cosmetic: function above adds empty line before return state and this isn't.
dkirillov commented 2023-05-19 08:20:33 +00:00
Member
Copy link

Maybe we can write:

	p.mu.RLock()
	defer p.mu.RUnlock()
	return p.defaultCopiesNumbers

The same way as in p.Default()?

Maybe we can write: ```golang p.mu.RLock() defer p.mu.RUnlock() return p.defaultCopiesNumbers ``` The same way as in `p.Default()`?
cmd/s3-gw/app.go Outdated
@ -472,0 +530,4 @@
a.log.Info("setting location constraint", zap.String("constraint", k), zap.Uint32s("vector", v))
}
} else {
a.log.Warn("location constraints won't be updated")
alexvanin commented 2023-05-19 06:57:57 +00:00
Owner
Copy link

Location constraints are optional in the config. So it is a valid case to remove all constraints and call SIGHUP (just to use default for all cases, for example).

Maybe we need to clear a map and refill it with new values there? The same way it is done in policies.update.

Location constraints are _optional_ in the config. So it is a valid case to remove all constraints and call SIGHUP (just to use default for all cases, for example). Maybe we need to clear a map and refill it with new values there? The same way it is done in `policies.update`.
dkirillov commented 2023-05-19 08:12:48 +00:00
Member
Copy link

I suggest change signature of fetchCopiesNumbers and make it return (map[string][]uint32, error). So if error is not nil we can log that this value won't be updated.

I suggest change signature of `fetchCopiesNumbers` and make it return `(map[string][]uint32, error)`. So if error is not `nil` we can log that this value won't be updated.
dkirillov reviewed 2023-05-19 08:17:34 +00:00
cmd/s3-gw/app.go Outdated
@ -160,2 +162,4 @@
}
var defaultCopiesNumbers []uint32
defaultCopiesNumbers = []uint32{handler.DefaultCopiesNumber}
dkirillov commented 2023-05-19 08:05:17 +00:00
Member
Copy link

Let's write:

defaultCopiesNumbers := []uint32{handler.DefaultCopiesNumber}
Let's write: ```golang defaultCopiesNumbers := []uint32{handler.DefaultCopiesNumber} ```
dkirillov marked this conversation as resolved
cmd/s3-gw/app.go Outdated
@ -162,0 +167,4 @@
defaultCopiesNumbers = val
}
policies.updateDefaultCopiesNumbers(defaultCopiesNumbers)
log.logger.Info("setting default copies numbers", zap.Uint32s("vector", policies.GetDefaultCopiesNumbers()))
dkirillov commented 2023-05-19 08:15:18 +00:00
Member
Copy link

We can write:

log.logger.Info("setting default copies numbers", zap.Uint32s("vector", defaultCopiesNumbers))
We can write: ```golang log.logger.Info("setting default copies numbers", zap.Uint32s("vector", defaultCopiesNumbers)) ```
ironbee commented 2023-05-19 08:39:57 +00:00
Author
Contributor
Copy link

Why shouldn't we refer to the actual content when logging this?

Why shouldn't we refer to the actual content when logging this?
👍 1
dkirillov marked this conversation as resolved
cmd/s3-gw/app.go Outdated
@ -162,0 +175,4 @@
log.logger.Info("settings location constraint", zap.String("location", k), zap.Uint32s("vector", v))
}
} else {
log.logger.Warn("location constraints are not set")
dkirillov commented 2023-05-19 08:07:15 +00:00
Member
Copy link

Actually, something like copies number be more accurate rather than location constraints

Actually, something like `copies number` be more accurate rather than `location constraints`
dkirillov marked this conversation as resolved
cmd/s3-gw/app.go Outdated
@ -472,0 +523,4 @@
a.log.Warn("default copies numbers won't be updated")
}
copiesNumbers := fetchCopiesNumbers(a.log, a.cfg)
dkirillov commented 2023-05-19 08:14:35 +00:00
Member
Copy link

Can we move both this updating to new method a.settings.policies.updateCopiesNumbers(...) similar to a.settings.policies.udpate(...)?

Can we move both this updating to new method `a.settings.policies.updateCopiesNumbers(...)` similar to `a.settings.policies.udpate(...)`?
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-05-19 08:23:45 +00:00
api/handler/api.go Outdated
@ -45,2 +43,4 @@
Default() netmap.PlacementPolicy
Get(string) (netmap.PlacementPolicy, bool)
GetCopiesNumbers(string) ([]uint32, bool)
GetDefaultCopiesNumbers() []uint32
dkirillov commented 2023-05-19 08:23:42 +00:00
Member
Copy link

Let's rename methods of this interface to:

PlacementPolicy interface {
		DefaultPolicy() netmap.PlacementPolicy				
		Policy(string) (netmap.PlacementPolicy, bool)
        DefaultCopiesNumbers() []uint32
		CopiesNumbers(string) ([]uint32, bool)
}

?

Let's rename methods of this interface to: ```golang PlacementPolicy interface { DefaultPolicy() netmap.PlacementPolicy Policy(string) (netmap.PlacementPolicy, bool) DefaultCopiesNumbers() []uint32 CopiesNumbers(string) ([]uint32, bool) } ``` ?
ironbee commented 2023-05-19 08:42:07 +00:00
Author
Contributor
Copy link

Why should we call methods with nouns without verbs?

Why should we call methods with nouns without verbs?
dkirillov commented 2023-05-19 08:51:46 +00:00
Member
Copy link

It's like a getter I suppose https://go.dev/doc/effective_go#Getters

It's like a getter I suppose https://go.dev/doc/effective_go#Getters
😕 1
ironbee commented 2023-05-19 09:23:42 +00:00
Author
Contributor
Copy link

If we rename Get to Policy then this line https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/src/branch/master/api/handler/put.go#L754 will look like h.cfg.Policy.Policy(...) which I don't feel well about.

If we rename `Get` to `Policy` then this line https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/src/branch/master/api/handler/put.go#L754 will look like `h.cfg.Policy.Policy(...)` which I don't feel well about.
dkirillov commented 2023-05-19 10:14:56 +00:00
Member
Copy link

We can use DefaultPlacementPolicy and PlacementPolicy methods names then.

@alexvanin What do you think about renaming methods in general?

We can use `DefaultPlacementPolicy` and `PlacementPolicy` methods names then. @alexvanin What do you think about renaming methods in general?
dkirillov marked this conversation as resolved
ironbee changed title from app: Reload copies numbers on SIGHUP to WIP: app: Reload copies numbers on SIGHUP 2023-05-22 16:20:21 +00:00
ironbee force-pushed update-copies-numbers-on-SIGHUP from 9096110388 to 046c30ba73 2023-05-22 16:24:26 +00:00 Compare
alexvanin approved these changes 2023-05-23 08:05:58 +00:00
alexvanin left a comment
Owner
Copy link

Overall LGTM.

Overall LGTM.
cmd/s3-gw/app.go Outdated
@ -345,0 +390,4 @@
p.mu.Unlock()
l.Info("default copies numbers", zap.Uint32s("vector", p.defaultCopiesNumbers))
return
} else {
alexvanin commented 2023-05-23 07:26:39 +00:00
Owner
Copy link

I don't think you need else block, because you have early return above.

I don't think you need `else` block, because you have early return above.
alexvanin marked this conversation as resolved
alexvanin reviewed 2023-05-23 08:20:01 +00:00
cmd/s3-gw/app.go Outdated
@ -345,0 +393,4 @@
} else {
l.Error("cannot parse default copies numbers", zap.Error(err))
if initialUpdate {
alexvanin commented 2023-05-23 08:20:01 +00:00
Owner
Copy link

I think you can avoid this flag if you set p.defaultCopiesNumbers in newPlacementPolicy function upfront.

Then you call updateDefaultCopiesNumbers and return error.

In newPlacementPolicy you get an error and log: default copies numbers.

In update you get an error and log: default copies numbers won't be updated`.

I think you can avoid this flag if you set `p.defaultCopiesNumbers` in `newPlacementPolicy` function upfront. Then you call `updateDefaultCopiesNumbers` and return error. In `newPlacementPolicy` you get an error and log: `default copies numbers`. In `update you get an error and log: `default copies numbers won't be updated`.
alexvanin marked this conversation as resolved
ironbee force-pushed update-copies-numbers-on-SIGHUP from 046c30ba73 to 926aea7054 2023-05-23 09:25:33 +00:00 Compare
ironbee force-pushed update-copies-numbers-on-SIGHUP from 926aea7054 to 96ec14c4b6 2023-05-23 09:37:03 +00:00 Compare
ironbee force-pushed update-copies-numbers-on-SIGHUP from 96ec14c4b6 to 48bb410275 2023-05-23 10:11:46 +00:00 Compare
ironbee changed title from WIP: app: Reload copies numbers on SIGHUP to app: Reload copies numbers on SIGHUP 2023-05-23 10:14:50 +00:00
ironbee force-pushed update-copies-numbers-on-SIGHUP from 48bb410275 to 9f186d9aba 2023-05-23 10:20:13 +00:00 Compare
alexvanin merged commit 9f186d9aba into master 2023-05-23 10:28:58 +00:00
alexvanin deleted branch update-copies-numbers-on-SIGHUP 2023-05-23 10:28:59 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
alexvanin
TrueCloudLab/storage-services-developers
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
good first issue

Good for newcomers

  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
good first issue
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
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-s3-gw#112
No description provided.
Delete branch "ironbee/frostfs-s3-gw:update-copies-numbers-on-SIGHUP"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?