-
Notifications
You must be signed in to change notification settings - Fork 458
[flink] Adding changes for flink 2.2 #2167
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
Conversation
|
@leonardBang , I made the changes. |
@vamossagar12 , please format the code with `mvn spotless:apply`. We can also compile before commit with `mvn clean install -DskipTests`
|
|
Yes, sorry not finding the time. Will do this in the next coulpe of days. |
fluss-flink/fluss-flink-2.2/src/main/java/org/apache/fluss/flink/catalog/Flink22Catalog.java
Outdated
Show resolved
Hide resolved
|
I also consider whether we need to move Flink21CatalogFactory to fluss-flink-common . Then flink 2.1 and 2.2 can share it. No need too much same code. FlinkCatalogFactory
FlinkCatalogGreeterThan21Factory
flink below 2.0 won't use FlinkCatalogGreeterThan21Factory code. Thus no problem will occurs. |
b7f48ba to
4096401
Compare
Thanks, I was thinking to do it in a subsequent PR. This PR also doesn't create an adapter layer for Flink 2.2,. we can adopt a similar approach for 2.1 in the common code. Let me know what you think. |
0079ad2 to
cfeafc8
Compare
|
There are some test failures, will take a look. |
|
@vamossagar12 I have add some adjusts to the code, you can add this to your commit.
|
530d962 to
bb651e2
Compare
1. merge flink21Catalog and flink22Catalog to FlinkCatalog. 2. add flink-2.2 to ci tests of flink
bb651e2 to
9e505e5
Compare
Currently, @wuchong @leonardBang , WDYT? |
+1 to delete 2.1 module and add support for 2.2, Flink 2.0 and 2.1 are rarely used bye users from my observation, maybe they're not stable enough as Flink's new major version, Flink 2.2 should be more stable than them. |
|
+1 to delete flink 2.1 We can do this just in this PR, making the support of Flink 2.2 as an upgrade from Flink 2.1 to Flink 2.2. |
|
Ok, thanks for the chnages @loserwang1024 . LGTM ! |
|
@leonardBang @wuchong flink 2.1 has been upgraded to flink 2.2, please help review |
wuchong
left a comment
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 @vamossagar12 and @loserwang1024 for this great work.
However, it seems Flink21MultipleParameterToolTest and Flink21MaterializedTableITCase are missed to move to Flink 2.2
fluss-flink/fluss-flink-2.2/src/main/java/org/apache/fluss/flink/adapter/FlinkSinkAdapter.java
Outdated
Show resolved
Hide resolved
...uss-flink-2.2/src/test/java/org/apache/fluss/flink/source/Flink22TableSourceBatchITCase.java
Outdated
Show resolved
Hide resolved
...nk/fluss-flink-2.2/src/test/java/org/apache/fluss/flink/source/Flink22TableSourceITCase.java
Show resolved
Hide resolved
...s-flink/fluss-flink-2.2/src/test/java/org/apache/fluss/flink/catalog/FlinkCatalog22Test.java
Outdated
Show resolved
Hide resolved
...s-flink/fluss-flink-2.2/src/test/java/org/apache/fluss/flink/catalog/FlinkCatalog22Test.java
Outdated
Show resolved
Hide resolved
Thanks for the review. I addressed the comments and added the 2 missing tests. PTAL. |
wuchong
left a comment
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.
LGTM.
I made another minor fix


Purpose
Linked issue: close #2101
Brief change log
Adding support for Flunk 2.2
Tests
Added bunch of unit and Integration tests.
API and Format
No
Documentation
Adds support for Flink 2.2. If we need documentation for this, can add in a separate PR.