Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enforce sharding defaults in period configs #2332

Merged
merged 4 commits into from
Mar 26, 2020
Merged

enforce sharding defaults in period configs #2332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5dca9a6
periodconfig rowshards defaults
owen-d Mar 25, 2020
2181c47
linting
owen-d Mar 25, 2020
ffc7fac
comment cleanup
owen-d Mar 25, 2020
af1490d
ensures schema validation in testware
owen-d Mar 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion pkg/chunk/fixtures.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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.
Expand Down
31 changes: 23 additions & 8 deletions pkg/chunk/schema_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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:
Expand All @@ -191,7 +189,13 @@ func (cfg PeriodConfig) createBucketsFunc() (schemaBucketsFunc, time.Duration) {
}
}

// validate the period config
func (cfg *PeriodConfig) applyDefaults() {
if cfg.RowShards == 0 {
cfg.RowShards = defaultRowShards(cfg.Schema)
}
}

// Validate the period config.
func (cfg PeriodConfig) validate() error {
// Ensure the schema version exists
schema := cfg.CreateSchema()
Expand All @@ -210,6 +214,17 @@ func (cfg PeriodConfig) validate() error {
return errInvalidTablePeriod
}

switch cfg.Schema {
case "v1", "v2", "v3", "v4", "v5", "v6", "v9":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if I guess it doesn't cause any harm, to avoid any misunderstanding/misconfiguration from the user side, shouldn't we ensure the cfg.RowShards for these schemas is 0? Am I missing anything?

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:
// This generally unreachable path protects us from adding schemas and not handling them in this function.
return fmt.Errorf("unexpected schema (%s)", cfg.Schema)
}

return nil
}

Expand Down
151 changes: 139 additions & 12 deletions pkg/chunk/schema_config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,19 +308,20 @@ 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{
Configs: []PeriodConfig{
{Schema: "v0"},
},
},
expected: errInvalidSchemaVersion,
err: errInvalidSchemaVersion,
},
"should fail on index table period not multiple of 1h for schema v1": {
config: &SchemaConfig{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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,
},
}

Expand All @@ -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)
}
})
}
}
Expand Down