Skip to content

fix calculation of expected tables and create tables from upcoming schema considering grace period #1976

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.

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

Merged
merged 3 commits into from
Jan 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ instructions below to upgrade your Postgres.
* [BUGFIX] TSDB: Fixed `cortex_ingester_queried_samples` and `cortex_ingester_queried_series` metrics when using block storage. #1981
* [BUGFIX] TSDB: Fixed `cortex_ingester_memory_series` and `cortex_ingester_memory_users` metrics when using with the experimental TSDB blocks storage. #1982
* [BUGFIX] TSDB: Fixed `cortex_ingester_memory_series_created_total` and `cortex_ingester_memory_series_removed_total` metrics when using TSDB blocks storage. #1990
* [BUGFIX] Table Manager: Fixed calculation of expected tables and creation of tables from next active schema considering grace period. #1976

### Upgrading Postgres (if you're using configs service)

Expand Down
4 changes: 2 additions & 2 deletions pkg/chunk/schema_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -424,8 +424,8 @@ func (cfg *PeriodicTableConfig) periodicTables(from, through model.Time, pCfg Pr
nowWeek = now / periodSecs
result = []TableDesc{}
)
// If through ends on 00:00 of the day, don't include the upcoming day
if through.Unix()%secondsInDay == 0 {
// If interval ends exactly on a period boundary, dont include the upcoming period
if through.Unix()%periodSecs == 0 {
lastTable--
}
// Don't make tables further back than the configured retention
Expand Down
3 changes: 2 additions & 1 deletion pkg/chunk/table_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,8 @@ func (m *TableManager) calculateExpectedTables() []TableDesc {
result := []TableDesc{}

for i, config := range m.schemaCfg.Configs {
if config.From.Time.Time().After(mtime.Now()) {
// Consider configs which we are about to hit and requires tables to be created due to grace period
if config.From.Time.Time().After(mtime.Now().Add(m.cfg.CreationGracePeriod)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to myself: this looks a safe change to me. We're going to create the new table a bit ahead of time, which is what we want.

continue
}
if config.IndexTables.Period == 0 { // non-periodic table
Expand Down
29 changes: 29 additions & 0 deletions pkg/chunk/table_manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,21 @@ func TestTableManager(t *testing.T) {
},
)

// Move ahead where we are short by just grace period before hitting next section
tmTest(t, client, tableManager,
"Move ahead where we are short by just grace period before hitting next section",
weeklyTable2Start.Add(-gracePeriod+time.Second),
[]TableDesc{
{Name: baseTableName, ProvisionedRead: inactiveRead, ProvisionedWrite: inactiveWrite},
{Name: tablePrefix + week1Suffix, ProvisionedRead: inactiveRead, ProvisionedWrite: inactiveWrite, WriteScale: inactiveScalingConfig},
{Name: tablePrefix + week2Suffix, ProvisionedRead: read, ProvisionedWrite: write, WriteScale: activeScalingConfig},
{Name: table2Prefix + "5", ProvisionedRead: read, ProvisionedWrite: write, WriteScale: activeScalingConfig},
{Name: chunkTablePrefix + week1Suffix, ProvisionedRead: inactiveRead, ProvisionedWrite: inactiveWrite},
{Name: chunkTablePrefix + week2Suffix, ProvisionedRead: read, ProvisionedWrite: write},
{Name: chunkTable2Prefix + "5", ProvisionedRead: read, ProvisionedWrite: write},
},
)

// Move to the next section of the config
tmTest(t, client, tableManager,
"Move forward to next section of schema config",
Expand Down Expand Up @@ -648,6 +663,20 @@ func TestTableManagerRetentionOnly(t *testing.T) {
},
)

// Check after three weeks and a day short by grace period, we have three tables (two previous periods and the new one), table 0 was deleted
tmTest(t, client, tableManager,
"Move forward by three table periods and a day short by grace period",
baseTableStart.Add(tablePeriod*3+24*time.Hour-gracePeriod),
[]TableDesc{
{Name: tablePrefix + "1", ProvisionedRead: inactiveRead, ProvisionedWrite: inactiveWrite, WriteScale: inactiveScalingConfig},
{Name: tablePrefix + "2", ProvisionedRead: inactiveRead, ProvisionedWrite: inactiveWrite, WriteScale: inactiveScalingConfig},
{Name: tablePrefix + "3", ProvisionedRead: read, ProvisionedWrite: write},
{Name: chunkTablePrefix + "1", ProvisionedRead: inactiveRead, ProvisionedWrite: inactiveWrite},
{Name: chunkTablePrefix + "2", ProvisionedRead: inactiveRead, ProvisionedWrite: inactiveWrite},
{Name: chunkTablePrefix + "3", ProvisionedRead: read, ProvisionedWrite: write},
},
)

// Verify that without RetentionDeletesEnabled no tables are removed
tableManager.cfg.RetentionDeletesEnabled = false
// Retention > 0 will prevent older tables from being created so we need to create the old tables manually for the test
Expand Down