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.

Sign up for GitHub

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

Jump to bottom

[FLINK-33980][core] Reorganize job configuration #24025

Closed
wants to merge 6 commits into from
Closed

[FLINK-33980][core] Reorganize job configuration #24025

wants to merge 6 commits into from

Conversation

JunRuiLee
Copy link
Contributor

What is the purpose of the change

Reorganize job configuration

Brief change log

  1. Migrate configuration from non-ConfigOption objects to use ConfigOption.
  2. Adopt a single Configuration object to house all configurations.
  3. Create complex Java objects, such as RestartBackoffTimeStrategyFactory, CheckpointStorage, and StateBackend, directly from the configuration on the JM side.

Verifying this change

This change is already covered by existing tests.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
  • The serializers: (yes / no / don't know)
  • The runtime per-record code paths (performance sensitive): (yes / no / don't know)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / no / don't know)
  • The S3 file system connector: (yes / no / don't know)

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

@flinkbot
Copy link
Collaborator

flinkbot commented Jan 4, 2024 •
edited

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

JunRuiLee
JunRuiLee commented Jan 4, 2024
View reviewed changes
Copy link
Contributor Author

@JunRuiLee JunRuiLee left a comment •
edited

Choose a reason for hiding this comment

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

The current description for the config option StateChangelogOptions.PERIODIC_MATERIALIZATION_INTERVAL in FLINK states that "The periodic materialization will be disabled when the value is negative."
However, FLINK does not allow setting a negative Duration value, which causes errors during the conversion between String types.

After discussing with @masteryhx offline, he will soon propose a fix to address this issue.
Before his solution is implemented, I have submitted a hotfix commit b99f1c9 to resolve related test failures. Once the fix is in place, this commit will be removed.

@1996fanrui 1996fanrui self-requested a review January 4, 2024 05:02
@JunRuiLee
Copy link
Contributor Author

@zhuzhurk Could you help to review this PR ? Thanks.

1996fanrui
1996fanrui reviewed Jan 5, 2024
View reviewed changes
Copy link
Member

@1996fanrui 1996fanrui left a comment

Choose a reason for hiding this comment

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

Thanks @JunRuiLee for the contribution!

FLINK-33935 improves 2 options related to statebackend and checkpoint. It is merged,and it has a little conflicts with this PR, please address it in your free time, thanks~

@JunRuiLee
Copy link
Contributor Author

Thanks @JunRuiLee for the contribution!

FLINK-33935 improves 2 options related to statebackend and checkpoint. It is merged,and it has a little conflicts with this PR, please address it in your free time, thanks~

Thanks @1996fanrui for the kind reminder. I have rebased this pull request and resolved the conflicts.

1996fanrui
1996fanrui reviewed Jan 5, 2024
View reviewed changes
Copy link
Member

@1996fanrui 1996fanrui left a comment

Choose a reason for hiding this comment

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

Thanks @JunRuiLee for the quick update!

I left some comments, please take a look in your free time, thanks~

flink-core/src/main/java/org/apache/flink/api/common/cache/DistributedCache.java Show resolved Hide resolved
flink-core/src/main/java/org/apache/flink/api/common/ExecutionConfig.java Outdated Show resolved Hide resolved
...-streaming-java/src/main/java/org/apache/flink/streaming/api/graph/StreamGraphGenerator.java Outdated Show resolved Hide resolved
...-streaming-java/src/main/java/org/apache/flink/streaming/api/graph/StreamGraphGenerator.java Outdated Show resolved Hide resolved
...ava/src/main/java/org/apache/flink/streaming/api/environment/StreamExecutionEnvironment.java Outdated Show resolved Hide resolved
@JunRuiLee
Copy link
Contributor Author

Thanks @1996fanrui for the quick reviews, I've updated this pr, PTAL~

1996fanrui
1996fanrui reviewed Jan 5, 2024
View reviewed changes
Copy link
Member

@1996fanrui 1996fanrui left a comment

Choose a reason for hiding this comment

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

Thanks @JunRuiLee for the quick update!

I only left one comment, please take a look in your free time, thanks~

1996fanrui
1996fanrui approved these changes Jan 5, 2024
View reviewed changes
Copy link
Member

@1996fanrui 1996fanrui left a comment

Choose a reason for hiding this comment

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

Thanks for the quick update!

LGTM assuming the CI is green.

@JunRuiLee
Copy link
Contributor Author

Updated this PR with a rebase onto the master branch and resolved conflicts.

@1996fanrui
Copy link
Member

Hi @JunRuiLee , would you mind rebaseing the master branch to fix the CI? thanks~

@JunRuiLee
Copy link
Contributor Author

Hi @JunRuiLee , would you mind rebaseing the master branch to fix the CI? thanks~

Hi @1996fanrui , I am currently waiting for the merge of #24046. Once that has been completed, I'll go ahead and remove the hotfix commit 6ba47965e234b6524f10dbe0d933e5e5dff6627, and then I will rebase onto the updated master branch. Thanks for your patience!

@1996fanrui 1996fanrui mentioned this pull request Jan 9, 2024
Closed
@masteryhx
Copy link
Contributor

Hi, @JunRuiLee, #24046 has been merged.
You could rebase the master and go ahead.

@JunRuiLee
Copy link
Contributor Author

Thanks @masteryhx for the heads up, I've updated the PR following the merge.

1996fanrui
1996fanrui approved these changes Jan 11, 2024
View reviewed changes
Copy link
Member

@1996fanrui 1996fanrui left a comment

Choose a reason for hiding this comment

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

Thanks @JunRuiLee for the rebase!

Hi @zhuzhurk , would you like to review this PR as well? If no any comments, I will merge this PR next Monday.

@zhuzhurk
Copy link
Contributor

Thanks @JunRuiLee for the rebase!

Hi @zhuzhurk , would you like to review this PR as well? If no any comments, I will merge this PR next Monday.

Thanks for helping with the review! @1996fanrui
I'd like to take another look before merging the changes.

zhuzhurk
zhuzhurk reviewed Jan 12, 2024
View reviewed changes
Copy link
Contributor

@zhuzhurk zhuzhurk left a comment

Choose a reason for hiding this comment

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

Thanks for creating this PR! @JunRuiLee
Nice work. It should be tough since I can see many struggle to ensure the compatibility.
I have a couple of comments. PTAL.

...ava/src/main/java/org/apache/flink/streaming/api/environment/StreamExecutionEnvironment.java Show resolved Hide resolved
...src/test/java/org/apache/flink/test/checkpointing/CheckpointAfterAllTasksFinishedITCase.java Outdated Show resolved Hide resolved
...src/test/java/org/apache/flink/streaming/api/environment/StreamExecutionEnvironmentTest.java Show resolved Hide resolved
...src/test/java/org/apache/flink/streaming/api/environment/StreamExecutionEnvironmentTest.java Show resolved Hide resolved
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/graph/StreamGraph.java Outdated Show resolved Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/dispatcher/Dispatcher.java Outdated Show resolved Hide resolved
...treaming-java/src/main/java/org/apache/flink/streaming/api/environment/CheckpointConfig.java Show resolved Hide resolved
...ache/flink/streaming/api/environment/StreamExecutionEnvironmentComplexConfigurationTest.java Outdated Show resolved Hide resolved
...ache/flink/streaming/api/environment/StreamExecutionEnvironmentComplexConfigurationTest.java Outdated Show resolved Hide resolved
...src/test/java/org/apache/flink/streaming/api/environment/StreamExecutionEnvironmentTest.java Outdated Show resolved Hide resolved
@JunRuiLee JunRuiLee force-pushed the Reorganize_job_configuration branch 2 times, most recently from 31ecf64 to 64930dc Compare January 13, 2024 09:51
@JunRuiLee
Copy link
Contributor Author

Thanks @zhuzhurk and @1996fanrui for your reviews, I've updated this pr accordingly, PTAL.

zhuzhurk
zhuzhurk approved these changes Jan 15, 2024
View reviewed changes
Copy link
Contributor

@zhuzhurk zhuzhurk left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all the comments. @JunRuiLee
LGTM.

@zhuzhurk zhuzhurk closed this in eb8af0c Jan 16, 2024
@JunRuiLee JunRuiLee deleted the Reorganize_job_configuration branch March 4, 2024 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@1996fanrui 1996fanrui 1996fanrui approved these changes

@zhuzhurk zhuzhurk zhuzhurk approved these changes

Assignees
No one assigned
Labels
component=API/Core
Projects
None yet
Milestone
No milestone
5 participants
@JunRuiLee @flinkbot @1996fanrui @masteryhx @zhuzhurk