Skip to content
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

Flink backend implementation #1629

Closed
wants to merge 16 commits into from
Closed

Conversation

pan3793
Copy link
Member

@pan3793 pan3793 commented Dec 24, 2021

Why are the changes needed?

This PR covers #1619.

Overall, this PR contains the following changs,

  1. change build/dist script to support flink sql engine
  2. enhance externals/flink-sql-engine/pom.xml to support create a shaded jar
  3. simplify externals/kyuubi-flink-sql-engine/bin/flink-sql-engine.sh
  4. introduce FlinkSQLEngine(flink sql engine entrypoint) and FlinkProcessBuilder(kyuubi server launcher)
  5. add ut in kyuubi server side

After this PR, we can run the basic query e.g. select now() from beeline and get result, and Kyuubi Server can auto launch flink engine if there is no proper one. The Flink engine also supports other engine share levels defined in Kyuubi.

The implementation based on Flink 1.14 codebase.

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

@codecov-commenter
Copy link

codecov-commenter commented Dec 27, 2021

Codecov Report

Merging #1629 (b7e5f0e) into master (54a1b88) will increase coverage by 0.85%.
The diff coverage is 57.76%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1629      +/-   ##
============================================
+ Coverage     58.23%   59.08%   +0.85%     
- Complexity      164      193      +29     
============================================
  Files           258      264       +6     
  Lines         12803    13079     +276     
  Branches       1608     1650      +42     
============================================
+ Hits           7456     7728     +272     
+ Misses         4717     4696      -21     
- Partials        630      655      +25     
Impacted Files Coverage Δ
...g/apache/kyuubi/engine/flink/result/Constants.java 0.00% <0.00%> (ø)
...org/apache/kyuubi/engine/flink/schema/RowSet.scala 50.00% <24.24%> (+36.13%) ⬆️
.../apache/kyuubi/engine/flink/FlinkEngineUtils.scala 29.41% <29.41%> (ø)
...rg/apache/kyuubi/engine/flink/FlinkSQLEngine.scala 42.10% <42.10%> (ø)
...yuubi/engine/flink/FlinkEngineProcessBuilder.scala 75.90% <75.90%> (ø)
...ain/scala/org/apache/kyuubi/engine/EngineRef.scala 86.00% <87.50%> (+1.05%) ⬆️
.../apache/kyuubi/engine/flink/result/ResultKind.java 100.00% <100.00%> (ø)
...e/kyuubi/engine/flink/FlinkSQLBackendService.scala 100.00% <100.00%> (ø)
...uubi/engine/flink/operation/ExecuteStatement.scala 80.64% <100.00%> (+12.90%) ⬆️
...kyuubi/engine/flink/operation/FlinkOperation.scala 55.00% <100.00%> (+12.37%) ⬆️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54a1b88...b7e5f0e. Read the comment docs.

@pan3793 pan3793 changed the title [WIP] Flink backend implementation Flink backend implementation Dec 27, 2021
build/dist Outdated
SUFFIX=""
else
SUFFIX="-spark-${SPARK_VERSION:0:3}-hadoop${SPARK_HADOOP_VERSION}"
if [[ "$NAME" != "none" ]]; then
Copy link
Member Author

Choose a reason for hiding this comment

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

build/dist --tgz --flink-provided --spark-provided => apache-kyuubi-1.5.0-SNAPSHOT-bin.tgz
build/dist --tgz --name custom => apache-kyuubi-1.5.0-SNAPSHOT-bin-custom.tgz
build/dist --tgz --spark-provided => apache-kyuubi-1.5.0-SNAPSHOT-bin-flink-1.14.tgz
build/dist --tgz --flink-provided => apache-kyuubi-1.5.0-SNAPSHOT-spark-3.1-hadoop3.2.tgz
build/dist --tgz => apache-kyuubi-1.5.0-SNAPSHOT-bin-flink-1.14-spark-3.1-hadoop3.2.tgz

Copy link
Member Author

Choose a reason for hiding this comment

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

Does this name pattern looks good?

Copy link
Contributor

Choose a reason for hiding this comment

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

currently, I am fine with the naming. But, if we introduce trino/presto, the name would be very long.

Copy link
Member

Choose a reason for hiding this comment

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

not good, we have a release file already

Copy link
Member Author

Choose a reason for hiding this comment

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

any suggestions?

Copy link
Member Author

Choose a reason for hiding this comment

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

not good, we have a release file already

We only have one apache official release binary tarball apache-kyuubi-{VERSION}-bin.tgz

Copy link
Member

Choose a reason for hiding this comment

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

let's leave it as-is, we don't change the naming policy with a new engine added

Copy link
Member Author

Choose a reason for hiding this comment

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

reverted

@pan3793 pan3793 self-assigned this Dec 27, 2021
@pan3793 pan3793 added the kind:feature Feature request label Dec 27, 2021
@pan3793 pan3793 added this to the v1.5.0 milestone Dec 27, 2021
Copy link
Contributor

@yanghua yanghua left a comment

Choose a reason for hiding this comment

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

LGTM

@yaooqinn
Copy link
Member

thanks, merged to master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature Feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants