-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Update prometheus dependency to commit #8836 #4247
Conversation
This testcase https://github.com/thanos-io/thanos/blob/main/pkg/store/bucket_test.go#L1938 is also broken due to the recent 120 chunks split change in Prometheus. Any idea about how to fix or change it? @metalmatze @bwplotka @onprem |
Hah, well looks like we relied on this ;p // This method relies on a bug in TSDB Compactor which will just merge overlapping chunks into one big chunk.// If compactor is fixed in the future, we may need a different way of generating the block, or commit// existing block to the repository. I think, maybe it's time to remove all tests relying on such large chunk situation, since. we hopefully don't have such? |
…3e45a31d5c Signed-off-by: yeya24 <yb532204897@gmail.com>
93be272
to
4ed9d50
Compare
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.
LGTM, just I am worried about other removed tests around Flush.. why?
pkg/store/tsdb_test.go
Outdated
@@ -684,49 +649,6 @@ func TestTSDBStore_SeriesAccessWithoutDelegateClosing(t *testing.T) { | |||
_ = string(c.Raw.Data) // Access bytes by converting them to different type. | |||
})) | |||
} | |||
testutil.NotOk(t, testutil.FaultOrPanicToErr(func() { |
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.
Why those are removed?
pkg/store/tsdb_test.go
Outdated
@@ -595,26 +580,6 @@ func TestTSDBStore_SeriesAccessWithDelegateClosing(t *testing.T) { | |||
// Let's close pending querier! | |||
testutil.Equals(t, 1, len(csrv.closers)) | |||
testutil.Ok(t, csrv.closers[0].Close()) | |||
|
|||
// Expect flush and close to be unblocked and without errors. |
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.
Why those are removed?
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.
hmmm prometheus/prometheus#8723 ... Can we skip them instead 🤔
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.
Yes, that's a better option.
Signed-off-by: yeya24 <yb532204897@gmail.com>
@bwplotka PR updated, let me know if I need to update anything else. |
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.
LGTM 🎉 🚀
Issue opened #4266. Will merge it. |
Signed-off-by: yeya24 yb532204897@gmail.com
Changes
This pr updates Prometheus dependency to prometheus/prometheus@0a89124.
Split from pr #4239.
This update included commit prometheus/prometheus#8723, which breaks the existing TSDB store test. The writability and readability are not deterministic so I removed most of the tests. Let me know if there is a better way to solve this.
Verification