-
Notifications
You must be signed in to change notification settings - Fork 145
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[FLINK-36648] Bump Flink version to Flink 2.0.0 #140
Conversation
@PatrickRen Would you like to review this PR when you have time? |
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.
@lvyanquan Thanks for the awesome contribution! I found more classes that can be removed.
I simply searched keyword deprecated
in the repo, and there are still some appearances. Could you check them all?
Also I see a lot of empty files under flink-connector-kafka/archunit-violations
. I think we should remove them as well.
...ector-kafka/src/main/java/org/apache/flink/streaming/connectors/kafka/KafkaContextAware.java
Outdated
Show resolved
Hide resolved
...ka/src/main/java/org/apache/flink/streaming/connectors/kafka/KafkaDeserializationSchema.java
Outdated
Show resolved
Hide resolved
...afka/src/main/java/org/apache/flink/streaming/connectors/kafka/KafkaSerializationSchema.java
Outdated
Show resolved
Hide resolved
...test/java/org/apache/flink/streaming/connectors/kafka/internals/KafkaTopicPartitionTest.java
Outdated
Show resolved
Hide resolved
...a/src/test/java/org/apache/flink/streaming/connectors/kafka/KafkaShortRetentionTestBase.java
Show resolved
Hide resolved
Remove some empty files, and remain some files which were referenced in stored.rules. |
E2e case would fail, as there is a bug in Collect Sink in 2.0-preview version. Should I delete or comment these cases out? WDYT @PatrickRen. |
Hi, @PatrickRen can you help to review again and help to complete this PR? |
6c08b62
to
f2bfcad
Compare
Rebase master to solve conflict. |
Hi @PatrickRen. I have made adjustments based on your previous suggestion. Can you help confirm if there are any other suggestions? |
.github/workflows/push_pr.yml
Outdated
flink: [ 1.20.0 ] | ||
jdk: [ '8, 11, 17, 21' ] | ||
flink: [ 2.0-preview1 ] | ||
jdk: [ '11, 17, 21' ] |
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.
Since Flink 2.0 is built on Java 17 can we simply build this there as well? Then we can remove Java 11 from the CI matrix
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.
Addressed it.
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 Flink is out of the preview version now right.
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.
Done.
Rebase and rearrange to make commit message more clearly. |
Hi, @PatrickRen @leonardBang @gyfora @prshnt could you take a look at this as Flink 2.0.0 was released recently. |
I hope this can be released soon! Would love to move to 2.0 for our project |
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.
Thanks @lvyanquan for the great work, LGTM
.../src/test/java/org/apache/flink/connector/kafka/dynamic/source/DynamicKafkaSourceITTest.java
Outdated
Show resolved
Hide resolved
@@ -30,7 +30,7 @@ jobs: | |||
strategy: | |||
matrix: | |||
flink_branches: [{ | |||
flink: 1.20-SNAPSHOT, | |||
flink: 2.1-SNAPSHOT, |
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.
what do you think about using the main branch with the stable release of Flink and not the snapshot?
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.
I tend to add both snapshot and stable versions to ensure compatibility, and this runs weekly with minimal resource overhead.
Thanks @lvyanquan for the PR and taking care of this. |
CI passed, merging... |
62091f4
to
7e14f5e
Compare
7e14f5e
to
76669bb
Compare
Awesome work, congrats on your first merged pull request! |
Bump Flink version to Flink 2.0-preview1.
This is required as flink-example module and flink-e2e-to-e2e-tests need to use Kafka connector for testing, and some other project like Apache Paimon also rely on Kafka connector in test.
This pr is based on #139 to remove many deprecated classes.
And as there is a bug in SinkTestSuiteBase/SourceTestSuiteBase in Flink 2.0-preview1 as https://issues.apache.org/jira/browse/FLINK-36845 described, temporarily commented relevant test as this will need to wait for a new release of Flink(Or we can directly cover these classes).