-
Notifications
You must be signed in to change notification settings - Fork 3.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
Fix flaky TestPrometheusIntegrationStatus.testPushDown #10778
Conversation
Additionally, set http-server.http.port as 8080 in the main method.
The interval might be shorter than 15s, so it returns 1 or 2 row.
@@ -100,8 +102,8 @@ public void testConfirmMetricAvailableAndCheckUp() | |||
@Test | |||
public void testPushDown() | |||
{ | |||
// default interval on the `up` metric that Prometheus records on itself is 15 seconds, so this should only yield one row | |||
// default interval on the `up` metric that Prometheus records on itself is about 15 seconds, so this should only yield one or two row | |||
MaterializedResult results = computeActual("SELECT * FROM prometheus.default.up WHERE timestamp > (NOW() - INTERVAL '15' SECOND)"); |
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.
Can we instead create a timestamp literal in Java side and then send that as the predicate.
In the MaterializedResult then we can assert that all rows have a timestamp > the predicate (instead of counting number of rows which is inherently flaky).
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.
Also if we want to test pushdown happenned we should assert on the plan. Since even without pushdown the engine will filter results and we'll get filtered rows.
It's pre-existing issue though.
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.
Let me punt to #10784
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.
Do we have some stress test results somewhere? LGTM.
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.
Tested locally because the failure ratio was very low (1 / 500 attempts) in my laptop. Verified 2000 passes with this fix.
Fixes #7224