Skip to content

Flink 1.15 - Fix flaky test that has implicit order dependency in TestFlinkTableSource#4697

Merged
RussellSpitzer merged 2 commits intoapache:masterfrom
kbendick:fix-flakey-limit-1-test-flink-115
May 6, 2022
Merged

Flink 1.15 - Fix flaky test that has implicit order dependency in TestFlinkTableSource#4697
RussellSpitzer merged 2 commits intoapache:masterfrom
kbendick:fix-flakey-limit-1-test-flink-115

Conversation

@kbendick
Copy link
Contributor

@kbendick kbendick commented May 5, 2022

Some tests in Flink's TestFlinkTableSource have been found to be order dependent.

As the order of results on read is not defined (without an order by clause), most of the tests were fixed in c726b22

However, there is one test that has a LIMIT 1 that then asserts on the row, which could be any of 3 possible rows.

As Flink happened to be returning the records in a stable manner prior to Flink 1.15, this test was passing. But in PR #4693, this test was discovered to occasionally fail, as SELECT * .... LIMIT 1 on a table with 3 results has no guarantee of which record will be returned.

Failed execution output:

org.apache.iceberg.flink.TestFlinkTableSource > testLimitPushDown FAILED
    java.lang.AssertionError: Should produce the expected records expected:<+I[1, iceberg, 10.0]> but was:<+I[2, b, 20.0]>
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:120)
        at org.apache.iceberg.flink.TestFlinkTableSource.testLimitPushDown(TestFlinkTableSource.java:107)

I've updated the test to assert that there is only one record, and that it is one of the 3 records in the table (which is assured by the previous query, which issues SELECT * .. LIMIT 4 with no filter and gets 3 records back).

@kbendick
Copy link
Contributor Author

kbendick commented May 5, 2022

cc @rdblue @openinx @stevenzwu @hililiwei @yittg this test is flaky (and technically always has been - but previously Flink was returning the records in a defined ordering).

Assert.assertEquals("Should have 1 record", 1, result.size());
Assertions.assertThat(result)
.containsAnyElementsOf(expectedList)
.size().isEqualTo(1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For people who cherry-pick, I'm not sure if this reordering is the best way to go about this.

I needed to use expectedList, so I just moved the test to be below the test that declares all 3 expected records of that table. Given that there are already multiple tests in this single test function, I think it should be fine.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure why we need the .size().isEqualTo(1); here when we checked that result.size() == 1 immediately before

Copy link
Member

@RussellSpitzer RussellSpitzer left a comment

Choose a reason for hiding this comment

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

I think the check on result size is redundant, but other than that looks good.

@kbendick
Copy link
Contributor Author

kbendick commented May 6, 2022

I think the check on result size is redundant, but other than that looks good.

Agreed. I left it in to make the changes as minimal as possible. I can remove it though.

@RussellSpitzer RussellSpitzer merged commit f79da86 into apache:master May 6, 2022
@RussellSpitzer
Copy link
Member

Thanks @kbendick for the patch!
Also thanks to @stevenzwu and @hililiwei for the reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants