-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
fix flaky retention tests #3976
fix flaky retention tests #3976
Conversation
@@ -66,7 +66,7 @@ var ( | |||
RowShards: 16, | |||
}, | |||
{ | |||
From: dayFromTime(start.Add(49 * time.Hour)), | |||
From: dayFromTime(start.Add(73 * time.Hour)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The schema start date for this and the previous one is too close with just a days gap. Based on when the tests run, when we add 24h
> duration < 48h
to start
, it could result in jumping ahead by 2 days and end up falling into this schema instead of the previous one. For example, if start
is 9th Jun 2300 hours
and we add 28h
like the test here, we would fall into Config[2]
as opposed to what the test expects i.e Config[1]
.
c.UserID, c.From, c.Through, matchers...) | ||
require.NoError(t.t, err) | ||
return len(chunks) == 1 && c.ExternalKey() == chunks[0].ExternalKey() | ||
return len(chunkIDs) == 1 && c.ExternalKey() == chunks[0].ExternalKey() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could get multiple chunk ids if the chunk overlaps multiple schema configs because the Cortex code dedupes chunk ids within a store, not across the stores. This change dedupes the chunk ids before checking for having single expected chunk id in the response.
@@ -66,7 +66,7 @@ var ( | |||
RowShards: 16, | |||
}, | |||
{ | |||
From: dayFromTime(start.Add(49 * time.Hour)), | |||
From: dayFromTime(start.Add(73 * time.Hour)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just some arbitrary value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to set the date to start + 3 days
with an extra hour for buffer. The extra buffer gets ignored because we drop the time and just use the day.
What this PR does / why we need it:
There are 2 flaky tests in retention which I am attempting to fix.
The tests fail based on when they run so this PR changes some tests by keeping that in mind to set the expectations right.
Checklist