From 4da3673a436b69068610d077f085a8e15bfa8af9 Mon Sep 17 00:00:00 2001 From: Sandeep Sukhani Date: Thu, 8 Aug 2019 16:34:52 +0530 Subject: [PATCH 1/4] added check for retention period to be a multiple of periodic table duration and relevant test Signed-off-by: Sandeep Sukhani --- CHANGELOG.md | 2 ++ pkg/chunk/table_manager.go | 6 ++++++ pkg/chunk/table_manager_test.go | 5 +++++ 3 files changed, 13 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c1804e5a0cb..edb9e4cab4b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ * [FEATURE] Add option to use jump hashing to load balance requests to memcached #1554 * [FEATURE] Add status page for HA tracker to distributors #1546 +* [CHANGE] Added check for retention period to be a multiple of periodic table duration #1564 ## 0.1.0 / 2019-08-07 @@ -13,3 +14,4 @@ * [FEATURE] You can specify "heap ballast" to reduce Go GC Churn #1489 * [BUGFIX] HA Tracker no longer always makes a request to Consul/Etcd when a request is not from the active replica #1516 * [BUGFIX] Queries are now correctly cancelled by the query-frontend #1508 +* [CHANGE] Added check for retention period to be a multiple of periodic table duration diff --git a/pkg/chunk/table_manager.go b/pkg/chunk/table_manager.go index 52b8534fdff..8a4a5be94c9 100644 --- a/pkg/chunk/table_manager.go +++ b/pkg/chunk/table_manager.go @@ -2,6 +2,7 @@ package chunk import ( "context" + "errors" "flag" "fmt" "sort" @@ -126,6 +127,11 @@ type TableManager struct { // NewTableManager makes a new TableManager func NewTableManager(cfg TableManagerConfig, schemaCfg SchemaConfig, maxChunkAge time.Duration, tableClient TableClient, objectClient BucketClient) (*TableManager, error) { + // Assume the newest config is the one to use for validation of retention + if cfg.RetentionPeriod != 0 && cfg.RetentionPeriod%schemaCfg.Configs[len(schemaCfg.Configs)-1].IndexTables.Period != 0 { + return nil, errors.New("retention period should now be a multiple of periodic table duration") + } + return &TableManager{ cfg: cfg, schemaCfg: schemaCfg, diff --git a/pkg/chunk/table_manager_test.go b/pkg/chunk/table_manager_test.go index e2704669ce2..a6951d7ab8f 100644 --- a/pkg/chunk/table_manager_test.go +++ b/pkg/chunk/table_manager_test.go @@ -691,4 +691,9 @@ func TestTableManagerRetentionOnly(t *testing.T) { {Name: chunkTablePrefix + "3", ProvisionedRead: read, ProvisionedWrite: write}, }, ) + + // Test table manager retention not multiple of periodic config + tbmConfig.RetentionPeriod++ + _, err = NewTableManager(tbmConfig, cfg, maxChunkAge, client, nil) + require.Error(t, err) } From aa4f38e568743313972c7ca53b1b0a7c09d31aa4 Mon Sep 17 00:00:00 2001 From: Sandeep Sukhani Date: Wed, 14 Aug 2019 17:20:07 +0530 Subject: [PATCH 2/4] updated changelog Signed-off-by: Sandeep Sukhani --- CHANGELOG.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index edb9e4cab4b..a5f60c72b1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ * [FEATURE] Add option to use jump hashing to load balance requests to memcached #1554 * [FEATURE] Add status page for HA tracker to distributors #1546 -* [CHANGE] Added check for retention period to be a multiple of periodic table duration #1564 +* [CHANGE] Retention period should now be a multiple of periodic table duration #1564 ## 0.1.0 / 2019-08-07 @@ -14,4 +14,3 @@ * [FEATURE] You can specify "heap ballast" to reduce Go GC Churn #1489 * [BUGFIX] HA Tracker no longer always makes a request to Consul/Etcd when a request is not from the active replica #1516 * [BUGFIX] Queries are now correctly cancelled by the query-frontend #1508 -* [CHANGE] Added check for retention period to be a multiple of periodic table duration From 18a4f18b2e331d98cca9eecaae1e9d77521676e0 Mon Sep 17 00:00:00 2001 From: Sandeep Sukhani Date: Wed, 14 Aug 2019 20:38:40 +0530 Subject: [PATCH 3/4] check for periodic table duration not 0 Signed-off-by: Sandeep Sukhani --- pkg/chunk/table_manager.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/pkg/chunk/table_manager.go b/pkg/chunk/table_manager.go index 8a4a5be94c9..3f9d8b6269d 100644 --- a/pkg/chunk/table_manager.go +++ b/pkg/chunk/table_manager.go @@ -127,9 +127,13 @@ type TableManager struct { // NewTableManager makes a new TableManager func NewTableManager(cfg TableManagerConfig, schemaCfg SchemaConfig, maxChunkAge time.Duration, tableClient TableClient, objectClient BucketClient) (*TableManager, error) { - // Assume the newest config is the one to use for validation of retention - if cfg.RetentionPeriod != 0 && cfg.RetentionPeriod%schemaCfg.Configs[len(schemaCfg.Configs)-1].IndexTables.Period != 0 { - return nil, errors.New("retention period should now be a multiple of periodic table duration") + + if cfg.RetentionPeriod != 0 { + // Assume the newest config is the one to use for validation of retention + indexTablesPeriod := schemaCfg.Configs[len(schemaCfg.Configs)-1].IndexTables.Period + if indexTablesPeriod != 0 && cfg.RetentionPeriod%indexTablesPeriod != 0 { + return nil, errors.New("retention period should now be a multiple of periodic table duration") + } } return &TableManager{ From 09469ad227fb92fb7a37ff4fd425d1421e1577f3 Mon Sep 17 00:00:00 2001 From: Sandeep Sukhani Date: Wed, 14 Aug 2019 20:39:07 +0530 Subject: [PATCH 4/4] moved update in changelog above features Signed-off-by: Sandeep Sukhani --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a5f60c72b1f..e75c17b4db4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,8 +1,8 @@ ## master / unreleased +* [CHANGE] Retention period should now be a multiple of periodic table duration #1564 * [FEATURE] Add option to use jump hashing to load balance requests to memcached #1554 * [FEATURE] Add status page for HA tracker to distributors #1546 -* [CHANGE] Retention period should now be a multiple of periodic table duration #1564 ## 0.1.0 / 2019-08-07