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-29014][streaming-java][table] Improve end-to-end story about PipelinesOptions.JARS #20633
Conversation
@lsyldliu could you help me with a review? Thanks :) |
Ok, I will review it as soon as possible. |
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.
Nice work and I only have one minor comment.
...able/flink-table-api-java/src/main/java/org/apache/flink/table/resource/ResourceManager.java
Outdated
Show resolved
Hide resolved
…t.getConfiguration StreamExecutionEnvironment.getConfiguration() was not only a ReadableConfig but also a deep copy. This caused various problems downstream (Python API) and makes accessing configuration expensive. Root configuration of TableConfig should not be a snapshot but a reference that always reflects the current status of StreamExecutionEnvironment. Otherwise, when merging e.g. PipelineOptions.JARS, it is possible that entries get lost when an outdated root configuration is used during merging.
…ipelinesOptions.JARS This closes apache#20633.
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.
+1
…ipelinesOptions.JARS This closes apache#20633.
What is the purpose of the change
This PR improves the configuration story in various aspects. The main target is to remove inconsistencies but also a simple architecture under the hood.
Brief change log
StreamExecutionEnvironment.getConfiguration() was not only a ReadableConfig but also a deep copy. This caused various problems downstream (Python API) and makes accessing configuration expensive. Root configuration of
TableConfig
should not be a snapshot but a reference that always reflects the current status of StreamExecutionEnvironment. Otherwise, when merging e.g.PipelineOptions.JARS
, it is possible that entries get lost when an outdated root configuration is used during merging.StreamExecutionEnvironment.configure() should not contain merging logic. It should be a set operation as all other options as well.
Table API should apply JAR file paths to TableConfig only. There is logic to apply this to lower layers already.
The same logic should also apply for StreamTableEnvironment.
Also,
PipelineOptions.JARS
is not set by default if no jars have been added.Verifying this change
This change added tests and can be verified as follows:
DataStreamJavaITCase#testResourcePropagation
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: yesDocumentation