Skip to content

[KYUUBI #4367] Support Flink 1.17#4368

Closed
link3280 wants to merge 42 commits intoapache:masterfrom
link3280:flink-1.17
Closed

[KYUUBI #4367] Support Flink 1.17#4368
link3280 wants to merge 42 commits intoapache:masterfrom
link3280:flink-1.17

Conversation

@link3280
Copy link
Contributor

@link3280 link3280 commented Feb 19, 2023

Why are the changes needed?

Support Flink 1.17 and Flink SQL gateway.

  1. Drop Flink 1.15
  2. Migrate API to Flink SQL Gateway
  3. Support Flink 1.17

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before make a pull request

@link3280
Copy link
Contributor Author

link3280 commented Feb 19, 2023

@bowenliang123
Copy link
Contributor

bowenliang123 commented Feb 19, 2023

Can you list here the breaking changes for the adaption for 1.17 and impacts for other versions?

@link3280
Copy link
Contributor Author

link3280 commented Feb 19, 2023

Note: The CI would NOT be successful because there are no flink 1.17 binaries on the apache site yet, plus I removed support for flink 1.16 or below temporarily. I will fix it when the flink 1.17 binaries are ready.

@link3280
Copy link
Contributor Author

link3280 commented Feb 19, 2023

Can you list out the main breaking changes and reasons of the adaption for 1.17?

Flink 1.17 refactors the SQL client module, adding a new module called Flink SQL gateway. Many classes Kyuubi relies on are refactored or relocated. For example, LocalExecutor and SessionContext.

WRT Flink 1.16 or below, surely I want to keep the support for at least 3 Flink minor versions, so I'm thinking of adding a new Flink engine module just for 1.17 or above if the community agrees.

@bowenliang123
Copy link
Contributor

Note: The CI would NOT be successful because there are no flink 1.17 binaries on the apache site yet, plus I removed support for flink 1.16 or below temporarily. I will fix it when the flink 1.17 binaries are ready.

Good to have a new profile and it job for flink 1.7. And we could test it in nighty workflows before it's released like we do to spark.

The versions of flink supported by kyuubi with this pr should be also clarified and discussed.

@pan3793
Copy link
Member

pan3793 commented Mar 18, 2023

@link3280 this PR also includes some code refactoring, we can extract them in separating PR and put them into master first. And after #4517, we can drop support for Flink 1.14 in master, this could reduce lots of code differences.

pan3793 added a commit that referenced this pull request Mar 24, 2023
### _Why are the changes needed?_

As discussed before, Kyuubi is going to support the latest 3 Flink versions, and to reduce the complexity of supporting Flink 1.17 #4368, we are going to remove support Flink 1.14 first.

### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [ ] Add screenshots for manual tests if appropriate

- [x] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request

Closes #4588 from pan3793/rm-flink-1.14.

Closes #4387

97d2633 [Cheng Pan] Remove support for Flink 1.14

Authored-by: Cheng Pan <chengpan@apache.org>
Signed-off-by: Cheng Pan <chengpan@apache.org>
@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2023

Codecov Report

Merging #4368 (8eb4da6) into master (5faebb1) will increase coverage by 0.36%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##             master    #4368      +/-   ##
============================================
+ Coverage     57.68%   58.04%   +0.36%     
  Complexity       13       13              
============================================
  Files           579      580       +1     
  Lines         32012    32235     +223     
  Branches       4275     4305      +30     
============================================
+ Hits          18465    18712     +247     
+ Misses        11784    11727      -57     
- Partials       1763     1796      +33     
Impacted Files Coverage Δ
...pache/kyuubi/engine/KyuubiApplicationManager.scala 78.66% <0.00%> (-1.34%) ⬇️
...ache/kyuubi/engine/flink/FlinkProcessBuilder.scala 75.23% <75.00%> (-1.24%) ⬇️

... and 29 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@link3280 link3280 changed the title [KYUUBI #4367] Support Flink 1.17 [WIP][KYUUBI #4367] Support Flink 1.17 Apr 10, 2023
@github-actions github-actions bot added the kind:infra license, community building, project builds, asf infra related, etc. label Apr 10, 2023
@pan3793
Copy link
Member

pan3793 commented Apr 12, 2023

Thanks @link3280, this approach looks good to me, even we must drop Flink 1.15

@link3280 link3280 changed the title [WIP][KYUUBI #4367] Support Flink 1.17 [KYUUBI #4367] Support Flink 1.17 Apr 15, 2023
@link3280 link3280 requested a review from pan3793 April 15, 2023 06:33
@link3280
Copy link
Contributor Author

Please kindly rerun the fail job @pan3793

@pan3793 pan3793 closed this in 553b2aa Apr 17, 2023
@pan3793 pan3793 added this to the v1.8.0 milestone Apr 17, 2023
@pan3793
Copy link
Member

pan3793 commented Apr 17, 2023

Thanks for the great work, merged to master for 1.8

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

Labels

kind:build kind:infra license, community building, project builds, asf infra related, etc. module:flink module:server

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants