Skip to content

[FLINK-37739][cli] fix flink-conf is not obtained in local mode #4006

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MOBIN-F
Copy link
Contributor

@MOBIN-F MOBIN-F commented Apr 28, 2025

current local mode lacks loading additionalJars and flinkConf
Local mode is convenient for development and debug
image

@MOBIN-F MOBIN-F changed the title [cli] fix flink-conf is not obtained in local mode [FLINK-37739][cli] fix flink-conf is not obtained in local mode Apr 28, 2025
Comment on lines 72 to 75
public static FlinkPipelineComposer ofMiniCluster() {
return new FlinkPipelineComposer(
StreamExecutionEnvironment.getExecutionEnvironment(), true);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it still necessary to keep this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are still many test cases that use this method. Since they do not use flinkConf and additionalJars, I chose to keep this method.

Comment on lines +273 to +276
@VisibleForTesting
public StreamExecutionEnvironment getEnv() {
return env;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to use Java reflection in the test to obtain this object, instead of creating a separate method for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reflection makes code difficult to maintain

Copy link
Contributor

@Mrart Mrart left a comment

Choose a reason for hiding this comment

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

Can we add test coverage to e2e test?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants