From 5dca9a60d4d74a9446cdf6b9b5f5b1206fe91f77 Mon Sep 17 00:00:00 2001 From: Owen Diehl Date: Wed, 25 Mar 2020 12:32:17 -0400 Subject: [PATCH 1/4] periodconfig rowshards defaults Signed-off-by: Owen Diehl --- pkg/chunk/schema_config.go | 30 +++++-- pkg/chunk/schema_config_test.go | 151 +++++++++++++++++++++++++++++--- 2 files changed, 162 insertions(+), 19 deletions(-) diff --git a/pkg/chunk/schema_config.go b/pkg/chunk/schema_config.go index bce069604da..e7569ae4c96 100644 --- a/pkg/chunk/schema_config.go +++ b/pkg/chunk/schema_config.go @@ -107,7 +107,9 @@ func (cfg *SchemaConfig) loadFromFile() error { // Validate the schema config and returns an error if the validation // doesn't pass func (cfg *SchemaConfig) Validate() error { - for _, periodCfg := range cfg.Configs { + for i, _ := range cfg.Configs { + periodCfg := &cfg.Configs[i] + periodCfg.applyDefaults() if err := periodCfg.validate(); err != nil { return err } @@ -142,10 +144,6 @@ func (cfg *SchemaConfig) ForEachAfter(t model.Time, f func(config *PeriodConfig) // CreateSchema returns the schema defined by the PeriodConfig func (cfg PeriodConfig) CreateSchema() Schema { - rowShards := defaultRowShards(cfg.Schema) - if cfg.RowShards > 0 { - rowShards = cfg.RowShards - } var e entries switch cfg.Schema { @@ -165,12 +163,12 @@ func (cfg PeriodConfig) CreateSchema() Schema { e = v9Entries{} case "v10": e = v10Entries{ - rowShards: rowShards, + rowShards: cfg.RowShards, } case "v11": e = v11Entries{ v10Entries: v10Entries{ - rowShards: rowShards, + rowShards: cfg.RowShards, }, } default: @@ -191,6 +189,13 @@ func (cfg PeriodConfig) createBucketsFunc() (schemaBucketsFunc, time.Duration) { } } +func (cfg *PeriodConfig) applyDefaults() { + // apply default shard values + if cfg.RowShards == 0 { + cfg.RowShards = defaultRowShards(cfg.Schema) + } +} + // validate the period config func (cfg PeriodConfig) validate() error { // Ensure the schema version exists @@ -210,6 +215,17 @@ func (cfg PeriodConfig) validate() error { return errInvalidTablePeriod } + switch cfg.Schema { + case "v1", "v2", "v3", "v4", "v5", "v6", "v9": + case "v10", "v11": + if cfg.RowShards == 0 { + return fmt.Errorf("Must have row_shards > 0 (current: %d) for schema (%s)", cfg.RowShards, cfg.Schema) + } + default: + // protects us from adding schemas and not updating this block + return fmt.Errorf("unexpected schema (%s)", cfg.Schema) + } + return nil } diff --git a/pkg/chunk/schema_config_test.go b/pkg/chunk/schema_config_test.go index f82a87a06db..a449d90c0a8 100644 --- a/pkg/chunk/schema_config_test.go +++ b/pkg/chunk/schema_config_test.go @@ -308,11 +308,12 @@ func TestSchemaConfig_Validate(t *testing.T) { tests := map[string]struct { config *SchemaConfig - expected error + expected *SchemaConfig + err error }{ "should pass the default config (ie. used cortex runs with a target not requiring the schema config)": { - config: &SchemaConfig{}, - expected: nil, + config: &SchemaConfig{}, + err: nil, }, "should fail on invalid schema version": { config: &SchemaConfig{ @@ -320,7 +321,7 @@ func TestSchemaConfig_Validate(t *testing.T) { {Schema: "v0"}, }, }, - expected: errInvalidSchemaVersion, + err: errInvalidSchemaVersion, }, "should fail on index table period not multiple of 1h for schema v1": { config: &SchemaConfig{ @@ -331,7 +332,7 @@ func TestSchemaConfig_Validate(t *testing.T) { }, }, }, - expected: errInvalidTablePeriod, + err: errInvalidTablePeriod, }, "should fail on chunk table period not multiple of 1h for schema v1": { config: &SchemaConfig{ @@ -343,7 +344,7 @@ func TestSchemaConfig_Validate(t *testing.T) { }, }, }, - expected: errInvalidTablePeriod, + err: errInvalidTablePeriod, }, "should pass on index and chunk table period multiple of 1h for schema v1": { config: &SchemaConfig{ @@ -355,7 +356,7 @@ func TestSchemaConfig_Validate(t *testing.T) { }, }, }, - expected: nil, + err: nil, }, "should fail on index table period not multiple of 24h for schema v10": { config: &SchemaConfig{ @@ -366,7 +367,7 @@ func TestSchemaConfig_Validate(t *testing.T) { }, }, }, - expected: errInvalidTablePeriod, + err: errInvalidTablePeriod, }, "should fail on chunk table period not multiple of 24h for schema v10": { config: &SchemaConfig{ @@ -378,7 +379,7 @@ func TestSchemaConfig_Validate(t *testing.T) { }, }, }, - expected: errInvalidTablePeriod, + err: errInvalidTablePeriod, }, "should pass on index and chunk table period multiple of 24h for schema v10": { config: &SchemaConfig{ @@ -390,7 +391,17 @@ func TestSchemaConfig_Validate(t *testing.T) { }, }, }, - expected: nil, + expected: &SchemaConfig{ + Configs: []PeriodConfig{ + { + Schema: "v10", + RowShards: 16, + IndexTables: PeriodicTableConfig{Period: 24 * time.Hour}, + ChunkTables: PeriodicTableConfig{Period: 24 * time.Hour}, + }, + }, + }, + err: nil, }, "should pass on index and chunk table period set to zero (no period tables)": { config: &SchemaConfig{ @@ -402,7 +413,54 @@ func TestSchemaConfig_Validate(t *testing.T) { }, }, }, - expected: nil, + expected: &SchemaConfig{ + Configs: []PeriodConfig{ + { + Schema: "v10", + RowShards: 16, + IndexTables: PeriodicTableConfig{Period: 0}, + ChunkTables: PeriodicTableConfig{Period: 0}, + }, + }, + }, + err: nil, + }, + "should set shard factor defaults": { + config: &SchemaConfig{ + Configs: []PeriodConfig{ + { + Schema: "v10", + }, + }, + }, + expected: &SchemaConfig{ + Configs: []PeriodConfig{ + { + Schema: "v10", + RowShards: 16, + }, + }, + }, + err: nil, + }, + "should not override explicit shard factor": { + config: &SchemaConfig{ + Configs: []PeriodConfig{ + { + Schema: "v11", + RowShards: 6, + }, + }, + }, + expected: &SchemaConfig{ + Configs: []PeriodConfig{ + { + Schema: "v11", + RowShards: 6, + }, + }, + }, + err: nil, }, } @@ -411,7 +469,76 @@ func TestSchemaConfig_Validate(t *testing.T) { t.Run(testName, func(t *testing.T) { actual := testData.config.Validate() - assert.Equal(t, testData.expected, actual) + assert.Equal(t, testData.err, actual) + if testData.expected != nil { + require.Equal(t, testData.expected, testData.config) + } + }) + } +} + +func TestPeriodConfig_Validate(t *testing.T) { + for _, tc := range []struct { + desc string + in PeriodConfig + err string + }{ + { + desc: "ignore pre v10 sharding", + in: PeriodConfig{ + + Schema: "v9", + IndexTables: PeriodicTableConfig{Period: 0}, + ChunkTables: PeriodicTableConfig{Period: 0}, + }, + }, + { + desc: "error on invalid schema", + in: PeriodConfig{ + + Schema: "v99", + IndexTables: PeriodicTableConfig{Period: 0}, + ChunkTables: PeriodicTableConfig{Period: 0}, + }, + err: "invalid schema version", + }, + { + desc: "v10 with shard factor", + in: PeriodConfig{ + + Schema: "v10", + RowShards: 16, + IndexTables: PeriodicTableConfig{Period: 0}, + ChunkTables: PeriodicTableConfig{Period: 0}, + }, + }, + { + desc: "v11 with shard factor", + in: PeriodConfig{ + + Schema: "v11", + RowShards: 16, + IndexTables: PeriodicTableConfig{Period: 0}, + ChunkTables: PeriodicTableConfig{Period: 0}, + }, + }, + { + desc: "error v10 no specified shard factor", + in: PeriodConfig{ + + Schema: "v10", + IndexTables: PeriodicTableConfig{Period: 0}, + ChunkTables: PeriodicTableConfig{Period: 0}, + }, + err: "Must have row_shards > 0 (current: 0) for schema (v10)", + }, + } { + t.Run(tc.desc, func(t *testing.T) { + if tc.err == "" { + require.Nil(t, tc.in.validate()) + } else { + require.Error(t, tc.in.validate(), tc.err) + } }) } } From 2181c47aa41f4a1c8562df5f2ccde42e50e75aea Mon Sep 17 00:00:00 2001 From: Owen Diehl Date: Wed, 25 Mar 2020 13:04:57 -0400 Subject: [PATCH 2/4] linting Signed-off-by: Owen Diehl --- pkg/chunk/schema_config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/chunk/schema_config.go b/pkg/chunk/schema_config.go index e7569ae4c96..d67d8aafedc 100644 --- a/pkg/chunk/schema_config.go +++ b/pkg/chunk/schema_config.go @@ -107,7 +107,7 @@ func (cfg *SchemaConfig) loadFromFile() error { // Validate the schema config and returns an error if the validation // doesn't pass func (cfg *SchemaConfig) Validate() error { - for i, _ := range cfg.Configs { + for i := range cfg.Configs { periodCfg := &cfg.Configs[i] periodCfg.applyDefaults() if err := periodCfg.validate(); err != nil { From ffc7faca90ca1ca78ac9f917514eea9ae2ce7499 Mon Sep 17 00:00:00 2001 From: Owen Diehl Date: Wed, 25 Mar 2020 13:15:22 -0400 Subject: [PATCH 3/4] comment cleanup Signed-off-by: Owen Diehl --- pkg/chunk/schema_config.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/chunk/schema_config.go b/pkg/chunk/schema_config.go index d67d8aafedc..924ecf88685 100644 --- a/pkg/chunk/schema_config.go +++ b/pkg/chunk/schema_config.go @@ -190,13 +190,12 @@ func (cfg PeriodConfig) createBucketsFunc() (schemaBucketsFunc, time.Duration) { } func (cfg *PeriodConfig) applyDefaults() { - // apply default shard values if cfg.RowShards == 0 { cfg.RowShards = defaultRowShards(cfg.Schema) } } -// validate the period config +// Validate the period config. func (cfg PeriodConfig) validate() error { // Ensure the schema version exists schema := cfg.CreateSchema() @@ -222,7 +221,7 @@ func (cfg PeriodConfig) validate() error { return fmt.Errorf("Must have row_shards > 0 (current: %d) for schema (%s)", cfg.RowShards, cfg.Schema) } default: - // protects us from adding schemas and not updating this block + // This generally unreachable path protects us from adding schemas and not handling them in this function. return fmt.Errorf("unexpected schema (%s)", cfg.Schema) } From af1490dcdb43dff9fd1c095a13a9fb62b75b69d9 Mon Sep 17 00:00:00 2001 From: Owen Diehl Date: Wed, 25 Mar 2020 13:31:38 -0400 Subject: [PATCH 4/4] ensures schema validation in testware Signed-off-by: Owen Diehl --- pkg/chunk/fixtures.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/chunk/fixtures.go b/pkg/chunk/fixtures.go index 807d738a38b..d15b0213b0d 100644 --- a/pkg/chunk/fixtures.go +++ b/pkg/chunk/fixtures.go @@ -35,7 +35,7 @@ var BenchmarkLabels = labels.Labels{ // DefaultSchemaConfig creates a simple schema config for testing func DefaultSchemaConfig(store, schema string, from model.Time) SchemaConfig { - return SchemaConfig{ + s := SchemaConfig{ Configs: []PeriodConfig{{ IndexType: store, Schema: schema, @@ -50,6 +50,10 @@ func DefaultSchemaConfig(store, schema string, from model.Time) SchemaConfig { }, }}, } + if err := s.Validate(); err != nil { + panic(err) + } + return s } // ChunksToMatrix converts a set of chunks to a model.Matrix.