-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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-25128][table] Reorganize table modules and introduce flink-table-planner-loader #18134
Conversation
9caf85b
to
5481a24
Compare
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 5481a24 (Thu Dec 16 14:43:28 UTC 2021) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
5481a24
to
a35a17e
Compare
187bd0d
to
c878f11
Compare
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 for this great PR @slinkydeveloper. It looks we are approaching. Here is my first set of feedback.
|
||
### Notes | ||
|
||
No module except `flink-table-planner` should depend on `flink-table-runtime` in production classpath, |
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.
"depending" is ok but only as provided
maybe you can rephrase this paragraph
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 rather prefer to keep it this way, just as a warning to everyone who even remotely thinks to add it as dependency :)
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.
But it needs to be added as a dependency. Otherwise you cannot run it in your IDE. Many people have problems with setting up Maven correctly. We should rephrase this to emphasize: provided
is fine
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.
For testing you need to depend on table-planner-loader, not on table-planner. I added a sentence about testing.
...nner-loader/src/main/java/org/apache/flink/table/planner/loader/DelegateExecutorFactory.java
Show resolved
Hide resolved
4fafb3d
to
ce0be79
Compare
…sloader in order to load the ParserFactory. This makes sure that it will try to load first from its own classpath, and then from the parent classpath. Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
…-table-api-java-uber to ship only api related packages in a single uber jar Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
…nt of the table-planner uber jar that includes scala, in order to be used by flink-table-planner-loader Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
… modules Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
…le-runtime, table-planner-loader and cep in lib, while table-planner_${scala.version} in opt. Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
…uffix. Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
…ts and examples Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
…enever is necessary and add a new e2e test to check both planners work correctly. Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
ce0be79
to
81ab49c
Compare
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.
Awesome work @slinkydeveloper. This change will help users a lot.
<include>org.scala-lang.modules:scala-parser-combinators_${scala.binary.version}</include> | ||
|
||
<!-- ReflectionsUtil --> | ||
<include>org.reflections:reflections</include> |
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.
Isn't this only used for tests? What is it used for? JSON plan?
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.
Check ReflectionsUtil
. It's used for the JSON plan but perhaps can be removed (not in this PR though)
<dependency> | ||
<groupId>org.apache.flink</groupId> | ||
<artifactId>flink-streaming-java</artifactId> | ||
<version>${project.version}</version> | ||
<scope>provided</scope> |
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.
Why not provided
anymore?
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.
Same as #18134 (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.
Actually, for this one I can remove it as we pull it from api-java-bridge
Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
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 given the build turns green 🥳
…lasspath Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
30db657
to
f39dfb3
Compare
…ons with runtime logic in runtime Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
…ct classloader Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
48601e3
to
787be2a
Compare
…necessary to check both planners This closes apache#18134.
…necessary to check both planners This closes apache#18134.
…necessary to check both planners This closes apache#18134.
…necessary to check both planners This closes apache#18134.
What is the purpose of the change
The goal of this PR is to allow arbitrary Scala versions in the user API, hiding the Scala version of the planner from the Scala version of the API. This required some changes in the module organization, as described in the changelog. I also included a README to give an overview to table api developers (not for end users) of the new package organization edf7350.
This PR fixes the following issues:
I will open a followup PR to take care of the documentation.
Changelog
PlannerBase
is now using the class' classloader in order to load theParserFactory
. In case of planner_${scala.version}, this essentially makes no difference, while in case of planner-loader it will try to load first from its own classpath (which isComponentClassLoader
), and then from the parent classpath.flink-table-uber
module and replaced withflink-table-api-uber
to ship only Java API related packages in a single uber jar.flink-table-runtime
ships janino and code-splitter in its uber jarflink-table-planner
pom has been reworked, removing dependencies shipped by flink-runtime and flink-dist like jackson and commons-lang3, and now it generates 2 uber jars: one is pretty much the same as we ship now on master, and the other, named loader-bundle, is a uber-jar which includes also scala, in order to be used by flink-table-planner-loaderflink-table-planner-loader
. This provides an implementation of the factories inorg.apache.flink.table.delegation
that uses an isolated classloader to load the implementations fromflink-table-planner
loader-bundle
JAR.flink-table-api-uber
,flink-table-runtime
,flink-table-planner-loader
andflink-cep
in/lib
, while we continue to shipflink-table-planner_${scala.version}
in/opt
, in order to allow users to replace it withflink-table-planner-loader
just in case they need it. This is the fundamental change for the user, and will documented in the followup doc PR.There are a couple of details worth to mention in order to understand all the moving parts of this PR:
ComponentClassLoader
used inPlannerModule
allows only certain classes to be loaded from its parent classloader. Those are all the classes starting with "org.apache.flink" (which includes relocated dependencies) plus the classes starting with one of the prefixes inownerClassPath
Verifying this change
A new e2e test has been included to check that both planner-loader and planner_${scala.version} works fine. All the previous e2e tests are now running with planner-loader, whenever is possible.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: mpDocumentation