Mitigate testDynamicFiltering* flakiness #18323

ssheikin · 2023-07-18T09:48:29Z

Mitigate testDynamicFiltering* flakiness

testDynamicFiltering* reuses global QueryTracker state which can
pruneExpiredQueries()
BaseJdbcConnectorTest is huge. Test methods are executed in parallel.
There could be more than query.max-history queries between
executeWithQueryId and SqlQueryManager.getFullQueryInfo invocations.

Executing testDynamicFiltering* methods even in almost single threaded
environment (executing these methods after all other in the test class)
eliminates this issue.

    private static final int LAST = 1000;
    @Test(priority = LAST)

However, community is not ready to introduce TestNG specific magic as
test ordering via priority.

Retrying assertions for unknown reason mitigates this issue as well.
So adding @flaky to make CI more solid.

Description

Additional context and related issues

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

kokosing · 2023-07-18T14:35:27Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

@@ -2026,6 +2035,7 @@ private void assertDynamicFiltering(@Language("SQL") String sql, JoinDistributio
                noDynamicFilteringResultWithQueryId.getResult(),
                "For query: \n " + sql);

+        // Flaky: Reuses global QueryTracker state which can pruneExpiredQueries()


if this assertion is flaky by its nature, then maybe we should retry in it or maybe we should use io.trino.testing.AbstractTestQueryFramework#executeExclusively(java.lang.Runnable)?

it's not flaky in traditional sense. The query history gets purged so once this assertion fails it will keep failing even on retries. the other option is to retry entire test (either via @Flaky or wrapping the entire test in assertEventually).

let's wrap test in assertEventually

assertEventually is widely used but it does not have maxRetries. 🤷
Does it worth to use failsafe here?

assertEventually has a timeout duration. it should be enough

I don't like usage of assertEventually because you retry entire test and not only wait for certain event that may happen eventually after the "when" phase of the test. In such this case I think usage of @Flaky is more justified.

I believe here using executeExclusively is the best as that eliminates impact of any other test.

assertEventually and @Flaky have the same logical meaning.
They retry test.
Without test retry the correct result will not be obtained eventually.
So assertEventually and @Flaky are equally incorrect.
I agree that executeExclusively would be a better fit. So I'll try that.

@kokosing executeExclusively would work only if all other tests which use the same runner would check for the lock too (e.g. obtain a read lock). This is not a case here.
Also we are interested in executeExclusively tests which require QueryTracker not only between themselves, but exclusively to all other tests as well.

protected void executeExclusively(Runnable executionBlock) { runner.getExclusiveLock().lock(); try { executionBlock.run(); } finally { runner.getExclusiveLock().unlock(); } }

So I'll stick with assertEventually implementation.

Also, as an alternative solution to choose I can propose to annotate methods with:

@Flaky @Test(priority = LAST)

@kokosing @hashhar @wendigo please choose what do you like more.

That's why I prefer this more: #18468

I was wrong about executeExclusively, so #18850

@kokosing executeExclusively would work only if all other tests which use the same runner would check for the lock too (e.g. obtain a read lock).

True.

This is not a case here.

Wrong. It IS a case here.
All methods of query runner check for this lock.

Also we are interested in executeExclusively tests which require QueryTracker not only between themselves, but exclusively to all other tests as well.

And it is, because All methods of query runner check for this lock.

kokosing · 2023-07-21T20:32:24Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

@@ -2017,6 +2020,19 @@ private void assertNoDynamicFiltering(@Language("SQL") String sql)
    }

    private void assertDynamicFiltering(@Language("SQL") String sql, JoinDistributionType joinDistributionType, boolean expectDynamicFiltering)
+    {
+        assertEventually(
+                new Duration(10, MINUTES),


10 minutes is a lot

I remember we had

Method io.trino.plugin.jdbc.BaseJdbcConnectorTest.testDynamicFiltering() didn't finish within the time-out 60000

that's why I've decided 10 minutes.

kokosing · 2023-07-21T20:38:22Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

@@ -2026,6 +2035,7 @@ private void assertDynamicFiltering(@Language("SQL") String sql, JoinDistributio
                noDynamicFilteringResultWithQueryId.getResult(),
                "For query: \n " + sql);

+        // Flaky: Reuses global QueryTracker state which can pruneExpiredQueries()


I don't like usage of assertEventually because you retry entire test and not only wait for certain event that may happen eventually after the "when" phase of the test. In such this case I think usage of @Flaky is more justified.

I believe here using executeExclusively is the best as that eliminates impact of any other test.

ssheikin · 2023-07-31T15:17:14Z

@kokosing @hashhar @wendigo please approve
or choose what do you like more #18323 (comment)

kokosing

% comment

kokosing · 2023-08-02T12:14:23Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

+     * This method relies on global state of QueryTracker. It may fail because of QueryTracker.pruneExpiredQueries()
+     * You must ensure that query was issued and this method invoked in isolation -
+     * which guarantees that there is less other queries between query creation and obtaining query info than `query.max-history`
+     */


Should you add TODO and mention #18499? I would do it.

todo added.

kokosing · 2023-08-02T12:15:08Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

@@ -1943,6 +1949,7 @@ public void testDynamicFilteringWithLimit()
                        "ON a.orderkey = b.orderkey AND b.totalprice < 1000");
    }

+    @Flaky(issue = "https://github.com/trinodb/trino/issues/18499", match = ".*SqlQueryManager.getFullQueryInfo.*")


Have you tested this match is truely solving the flakiness?

Yes. 20/20 runs passed.

@test

`testDynamicFiltering*` reuses global `QueryTracker` state which can `pruneExpiredQueries()` `BaseJdbcConnectorTest` is huge. Test methods are executed in parallel. There could be more than `query.max-history` queries between `executeWithQueryId` and `SqlQueryManager.getFullQueryInfo` invocations. Executing `testDynamicFiltering*` methods even in almost single threaded environment (executing these methods after all other in the test class) eliminates this issue. ``` private static final int LAST = 1000; @test(priority = LAST) ``` However, community is not ready to introduce TestNG specific magic as test ordering via `priority`. Retrying assertions for unknown reason mitigates this issue as well. So adding @flaky to make CI more solid.

ssheikin · 2023-08-02T15:42:19Z

@kokosing all tests have passed. Please merge.

kokosing · 2023-08-02T18:57:15Z

Thank you!

cla-bot bot added the cla-signed label Jul 18, 2023

ssheikin requested review from kokosing, wendigo and hashhar July 18, 2023 09:49

kokosing reviewed Jul 18, 2023

View reviewed changes

ssheikin force-pushed the ssheikin/38/trino/flaky_testDynamicFiltering branch 2 times, most recently from 929ca1b to 66eefab Compare July 21, 2023 16:41

kokosing reviewed Jul 21, 2023

View reviewed changes

ssheikin force-pushed the ssheikin/38/trino/flaky_testDynamicFiltering branch from 66eefab to e111df0 Compare July 31, 2023 15:07

ssheikin mentioned this pull request Jul 31, 2023

Mitigate testDynamicFiltering* flakiness 2 #18468

Closed

kokosing approved these changes Aug 1, 2023

View reviewed changes

ssheikin mentioned this pull request Aug 2, 2023

Rewrite testDynamicFiltering* to run in isolation to mitigate flakiness #18499

Closed

ssheikin force-pushed the ssheikin/38/trino/flaky_testDynamicFiltering branch 2 times, most recently from c0f5efb to 0e5f7e1 Compare August 2, 2023 11:56

kokosing approved these changes Aug 2, 2023

View reviewed changes

kokosing reviewed Aug 2, 2023

View reviewed changes

ssheikin force-pushed the ssheikin/38/trino/flaky_testDynamicFiltering branch from 0e5f7e1 to 05889f5 Compare August 2, 2023 13:00

kokosing merged commit f3c41a4 into trinodb:master Aug 2, 2023

github-actions bot added this to the 423 milestone Aug 2, 2023

ssheikin deleted the ssheikin/38/trino/flaky_testDynamicFiltering branch August 2, 2023 20:58

mosabua mentioned this pull request Aug 3, 2023

Add Trino 423 release notes #18496

Merged

ssheikin mentioned this pull request Aug 29, 2023

Execute testDynamicFiltering* exclusively #18850

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mitigate testDynamicFiltering* flakiness #18323

Mitigate testDynamicFiltering* flakiness #18323

ssheikin commented Jul 18, 2023 •

edited

Loading

kokosing Jul 18, 2023

hashhar Jul 19, 2023

wendigo Jul 19, 2023

ssheikin Jul 19, 2023

wendigo Jul 19, 2023

kokosing Jul 21, 2023

ssheikin Jul 22, 2023

ssheikin Jul 31, 2023

ssheikin Jul 31, 2023

ssheikin Aug 29, 2023

kokosing Jul 21, 2023

ssheikin Jul 22, 2023

kokosing Jul 21, 2023

ssheikin commented Jul 31, 2023

kokosing left a comment

kokosing Aug 2, 2023

ssheikin Aug 2, 2023

kokosing Aug 2, 2023

ssheikin Aug 2, 2023

kokosing Aug 2, 2023

ssheikin commented Aug 2, 2023

kokosing commented Aug 2, 2023

Mitigate testDynamicFiltering* flakiness #18323

Mitigate testDynamicFiltering* flakiness #18323

Conversation

ssheikin commented Jul 18, 2023 • edited Loading

Description

Additional context and related issues

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssheikin commented Jul 31, 2023

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssheikin commented Aug 2, 2023

kokosing commented Aug 2, 2023

ssheikin commented Jul 18, 2023 •

edited

Loading