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
DefaultMaxAge int
NotificatorEnabled bool
CopiesNumber uint32
DefaultCopiesNumbers []uint32
CopiesNumbers map[string][]uint32
ResolveZoneList []string
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:
// 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
// 3) default copies number from the config file wrapped into array
// 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.
// 3) default copies number from the config file wrapped into array.
func (h *handler) pickCopiesNumbers(metadata map[string]string, locationConstraint string) ([]uint32, error) {
copiesNumbersStr, ok := metadata[layer.AttributeFrostfsCopiesNumber]
if ok {
@ -102,7 +102,7 @@ func (h *handler) pickCopiesNumbers(metadata map[string]string, locationConstrai
return copiesNumbers, nil
}
return []uint32{h.cfg.CopiesNumber}, nil
return h.cfg.DefaultCopiesNumbers, nil
}
func parseCopiesNumbers(copiesNumbersStr string) ([]uint32, error) {

View file

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

View file

@ -637,7 +637,7 @@ func (a *App) initHandler() {
Policy: a.settings.policies,
DefaultMaxAge: handler.DefaultMaxAge,
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,
}
@ -654,9 +654,10 @@ func (a *App) initHandler() {
cfg.CopiesNumbers = fetchCopiesNumbers(a.log, a.cfg)
if val := a.cfg.GetUint32(cfgSetCopiesNumber); val > 0 {
cfg.CopiesNumber = val
if val := fetchDefaultCopiesNumbers(a.log, a.cfg); len(val) > 0 {
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.IsResolveListAllow = len(cfg.ResolveZoneList) > 0

View file

@ -152,6 +152,22 @@ var ignore = map[string]struct{}{
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 {
var copiesNums = make(map[string][]uint32)
LOOP:
@ -160,6 +176,10 @@ LOOP:
constraint := v.GetString(key + "location_constraint")
vector := v.GetStringSlice(key + "vector")
if constraint == "" || len(vector) == 0 {
break
}
vector32 := make([]uint32, len(vector))
for j := range vector {
parsedValue, err := strconv.ParseUint(vector[j], 10, 32)
@ -170,10 +190,6 @@ LOOP:
vector32[j] = uint32(parsedValue)
}
if constraint == "" || len(vector) == 0 {
break
}
copiesNums[constraint] = vector32
l.Debug("added constraint", zap.String("location", constraint), zap.Strings("copies numbers", vector))
}