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-13447][table-api] Change default planner to legacy planner instead of any one #9249
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 4ac3a6f (Tue Aug 06 15:53:49 UTC 2019) 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:
|
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.
Hi @wuchong . Thank you for the PR. I put one comment regarding javadocs.
private static final String BLINK_EXECUTOR_FACTORY = "org.apache.flink.table.planner.delegation.BlinkExecutorFactory"; | ||
|
||
private String plannerClass = OLD_PLANNER_FACTORY; | ||
private String executorClass = OLD_EXECUTOR_FACTORY; | ||
private String builtInCatalogName = "default_catalog"; | ||
private String builtInDatabaseName = "default_database"; | ||
private boolean isStreamingMode = true; | ||
|
||
/** | ||
* Sets the old Flink planner as the required module. By default, {@link #useAnyPlanner()} is |
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.
Please update the javadoc: By default, {@link #useAnyPlanner()} is
-> By default, {@link #useOldPlanner()} is
. Also in the remaining two methods.
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.
Good catch!
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 think it's good to go now.
Travis passed: https://travis-ci.org/wuchong/flink/builds/564895873 Merging |
… of any one This closes #9249
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.
Nevermind, my Github view was not updated.
… of any one This closes apache#9249
What is the purpose of the change
We want to change the default behavior of the
EnvironmentSettings
to use old planner instead of any planner. This will enable us to have both planner in the classpath by default. This will also enable users/connectors to have both planner in dependency and without using EnvironmentSettings explicitly.Brief change log
Update
EnvironmentSettings
to use old planner instead of any planner by default.Verifying this change
This change is a trivial rework without any test coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation