Skip to content

Conversation

@openinx
Copy link
Member

@openinx openinx commented Nov 2, 2021

I think this PR: #3364 forgot to run the common flink unit tests for the given specific flink version. So when I run the following command , it even did not run any test cases.

./gradlew  -DflinkVersions=1.13 :iceberg-flink:iceberg-flink-1.13:build -Pquick=true -x javadoc

This RP is trying to enable the unit tests check for different flink versions.

@openinx openinx requested a review from rdblue November 2, 2021 03:42
@openinx openinx self-assigned this Nov 2, 2021
@openinx
Copy link
Member Author

openinx commented Nov 2, 2021

I found this because:

  1. When checking the flip-27 source reader PR from @stevenzwu , the current CI did not detect the missing of newly introduced flink dependencies for flink 1.12 and flink1.3. https://github.com/apache/iceberg/pull/2305/files#r740086825
  2. In this PR: Flink: Support flink 1.14.0 #3434, it just pass all the check for flink 1.14, but in fact we did nothing when run the test cases.

@stevenzwu
Copy link
Contributor

@openinx this looks good to me.

On a separate note, is iceberg-flink-runtime module still needed after the multi-version support change?

@openinx
Copy link
Member Author

openinx commented Nov 2, 2021

@stevenzwu we don't need the common iceberg-flink-runtime any more, instead we provided a separate flink runtime for different versions, such as iceberg-flink-1.12-runtime, iceberg-flin-1.13-runtime.

@stevenzwu
Copy link
Contributor

@rdblue can you take a look at this PR and merge if it looks good to you?

@rdblue
Copy link
Contributor

rdblue commented Nov 3, 2021

Since the tests are running in both 1.12 and 1.13 jobs, we no longer need the flink-common-tests CI job. Can you remove it?

@github-actions github-actions bot added the INFRA label Nov 4, 2021
@openinx
Copy link
Member Author

openinx commented Nov 5, 2021

As we are planning to separate the code base for flink 1.12 , flink 1.13, flink 1.14 ( From this comment1, comment2, comment3 ). We don't need this unit tests change now. Let's close it.

@openinx openinx closed this Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants