app: Refactor the default copies number setting #109

Merged
alexvanin merged 1 commit from ironbee/frostfs-s3-gw:refactor_set-copies-number into master 2023-05-17 12:43:03 +00:00
4 changed files with 35 additions and 18 deletions

View file

@ -34,7 +34,7 @@ type (
XMLDecoder XMLDecoderProvider XMLDecoder XMLDecoderProvider
DefaultMaxAge int DefaultMaxAge int
NotificatorEnabled bool NotificatorEnabled bool
CopiesNumber uint32 DefaultCopiesNumbers []uint32
CopiesNumbers map[string][]uint32 CopiesNumbers map[string][]uint32
ResolveZoneList []string ResolveZoneList []string
IsResolveListAllow bool // True if ResolveZoneList contains allowed zones IsResolveListAllow bool // True if ResolveZoneList contains allowed zones
@ -84,9 +84,9 @@ func New(log *zap.Logger, obj layer.Client, notificator Notificator, cfg *Config
} }
// pickCopiesNumbers chooses the return values following this logic: // pickCopiesNumbers chooses the return values following this logic:
// 1) array of copies numbers sent in request's header has the highest priority // 1) array of copies numbers sent in request's header has the highest priority.
// 2) array of copies numbers with corresponding location constraint provided in the config file // 2) array of copies numbers with corresponding location constraint provided in the config file.
// 3) default copies number from the config file wrapped into array // 3) default copies number from the config file wrapped into array.
func (h *handler) pickCopiesNumbers(metadata map[string]string, locationConstraint string) ([]uint32, error) { func (h *handler) pickCopiesNumbers(metadata map[string]string, locationConstraint string) ([]uint32, error) {
copiesNumbersStr, ok := metadata[layer.AttributeFrostfsCopiesNumber] copiesNumbersStr, ok := metadata[layer.AttributeFrostfsCopiesNumber]
if ok { if ok {
@ -102,7 +102,7 @@ func (h *handler) pickCopiesNumbers(metadata map[string]string, locationConstrai
return copiesNumbers, nil return copiesNumbers, nil
} }
return []uint32{h.cfg.CopiesNumber}, nil return h.cfg.DefaultCopiesNumbers, nil
} }
func parseCopiesNumbers(copiesNumbersStr string) ([]uint32, error) { func parseCopiesNumbers(copiesNumbersStr string) ([]uint32, error) {

View file

@ -13,8 +13,8 @@ func TestCopiesNumberPicker(t *testing.T) {
locConstraints[locationConstraint1] = []uint32{2, 3, 4} locConstraints[locationConstraint1] = []uint32{2, 3, 4}
config := &Config{ config := &Config{
CopiesNumber: 1, DefaultCopiesNumbers: []uint32{1},
CopiesNumbers: locConstraints, CopiesNumbers: locConstraints,
} }
h := handler{ h := handler{
cfg: config, cfg: config,

View file

@ -634,11 +634,11 @@ func getAccessBoxCacheConfig(v *viper.Viper, l *zap.Logger) *cache.Config {
func (a *App) initHandler() { func (a *App) initHandler() {
cfg := &handler.Config{ cfg := &handler.Config{
Policy: a.settings.policies, Policy: a.settings.policies,
DefaultMaxAge: handler.DefaultMaxAge, DefaultMaxAge: handler.DefaultMaxAge,
NotificatorEnabled: a.cfg.GetBool(cfgEnableNATS), NotificatorEnabled: a.cfg.GetBool(cfgEnableNATS),
CopiesNumber: handler.DefaultCopiesNumber, DefaultCopiesNumbers: []uint32{handler.DefaultCopiesNumber},
dkirillov marked this conversation as resolved Outdated

Let's use []uint32{handler.DefaultCopiesNumber} or drop handler.DefaultCopiesNumber.
Probably we can skip initialization of this field because empty/nil vector leads to default behavior (the same as vectore with one element 0)

Let's use `[]uint32{handler.DefaultCopiesNumber}` or drop `handler.DefaultCopiesNumber`. Probably we can skip initialization of this field because empty/nil vector leads to default behavior (the same as vectore with one element `0`)
XMLDecoder: a.settings.xmlDecoder, XMLDecoder: a.settings.xmlDecoder,
} }
if a.cfg.IsSet(cfgDefaultMaxAge) { if a.cfg.IsSet(cfgDefaultMaxAge) {
@ -654,9 +654,10 @@ func (a *App) initHandler() {
cfg.CopiesNumbers = fetchCopiesNumbers(a.log, a.cfg) cfg.CopiesNumbers = fetchCopiesNumbers(a.log, a.cfg)
if val := a.cfg.GetUint32(cfgSetCopiesNumber); val > 0 { if val := fetchDefaultCopiesNumbers(a.log, a.cfg); len(val) > 0 {
cfg.CopiesNumber = val cfg.DefaultCopiesNumbers = val
} }
a.log.Info("setting default copies numbers", zap.Uint32s("vector", cfg.DefaultCopiesNumbers))

Let's change Debug to Info

Let's change `Debug` to `Info`

Why shouldn't it be the debug?

Why shouldn't it be the `debug`?

Info will be more consistent with other similar cases. And it will be more convenient to see this information about copies number without any changes on cluster (where logging level is info)

`Info` will be more consistent with [other](https://git.frostfs.info/ironbee/frostfs-s3-gw/src/commit/e57795bb9991329106e80cfa8c9043a766744119/cmd/s3-gw/app.go#L239) [similar](https://git.frostfs.info/ironbee/frostfs-s3-gw/src/commit/e57795bb9991329106e80cfa8c9043a766744119/cmd/s3-gw/app.go#L381) [cases](https://git.frostfs.info/ironbee/frostfs-s3-gw/src/commit/e57795bb9991329106e80cfa8c9043a766744119/cmd/s3-gw/app.go#L123). And it will be more convenient to see this information about copies number without any changes on cluster (where logging level is `info`)

The same for #109 (comment)

The same for https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/pulls/109#issuecomment-9639
cfg.ResolveZoneList = a.cfg.GetStringSlice(cfgResolveBucketAllow) cfg.ResolveZoneList = a.cfg.GetStringSlice(cfgResolveBucketAllow)
cfg.IsResolveListAllow = len(cfg.ResolveZoneList) > 0 cfg.IsResolveListAllow = len(cfg.ResolveZoneList) > 0

View file

@ -152,6 +152,22 @@ var ignore = map[string]struct{}{
cmdVersion: {}, cmdVersion: {},
} }
func fetchDefaultCopiesNumbers(l *zap.Logger, v *viper.Viper) []uint32 {
unparsed := v.GetStringSlice(cfgSetCopiesNumber)
var result []uint32
for i := range unparsed {
parsedValue, err := strconv.ParseUint(unparsed[i], 10, 32)
if err != nil {
l.Error("cannot parse default copies numbers", zap.Error(err))
return make([]uint32, 0)
alexvanin marked this conversation as resolved Outdated

What you think about returning empty slice (i.e. default) in case of an error during parsing? I think this is more appropriate than having half parsed slice.

What you think about returning empty slice (i.e. default) in case of an error during parsing? I think this is more appropriate than having half parsed slice.
}
result = append(result, uint32(parsedValue))
}
return result
}
func fetchCopiesNumbers(l *zap.Logger, v *viper.Viper) map[string][]uint32 { func fetchCopiesNumbers(l *zap.Logger, v *viper.Viper) map[string][]uint32 {
var copiesNums = make(map[string][]uint32) var copiesNums = make(map[string][]uint32)
LOOP: LOOP:
@ -160,6 +176,10 @@ LOOP:
constraint := v.GetString(key + "location_constraint") constraint := v.GetString(key + "location_constraint")
vector := v.GetStringSlice(key + "vector") vector := v.GetStringSlice(key + "vector")
if constraint == "" || len(vector) == 0 {
break
}
vector32 := make([]uint32, len(vector)) vector32 := make([]uint32, len(vector))
for j := range vector { for j := range vector {
parsedValue, err := strconv.ParseUint(vector[j], 10, 32) parsedValue, err := strconv.ParseUint(vector[j], 10, 32)
@ -170,10 +190,6 @@ LOOP:
vector32[j] = uint32(parsedValue) vector32[j] = uint32(parsedValue)
} }
if constraint == "" || len(vector) == 0 {
break
}
copiesNums[constraint] = vector32 copiesNums[constraint] = vector32
l.Debug("added constraint", zap.String("location", constraint), zap.Strings("copies numbers", vector)) l.Debug("added constraint", zap.String("location", constraint), zap.Strings("copies numbers", vector))
} }