Skip to content

Conversation

@lincoln-lil
Copy link
Contributor

@lincoln-lil lincoln-lil commented Mar 15, 2024

  • Update Dockerfiles for 1.19.0 release

Copy link
Contributor

@MartijnVisser MartijnVisser left a 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, left one comment to double check

chown -R flink:flink .; \
\
# Replace default REST/RPC endpoint bind address to use the container's network interface \
CONF_FILE="$FLINK_HOME/conf/flink-conf.yaml"; \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still correct, given the new YAML parser?

Copy link
Contributor Author

@lincoln-lil lincoln-lil Mar 16, 2024

Choose a reason for hiding this comment

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

Good reminder! Seems there's a fallback logic here. I'll check the code and also dist package to verify 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.

I checked both the FLINK-33297 and FLIP-366, the default configuration file has been changed to config.yaml, but the script is compatible with the old flink-conf.yaml file.
This is FLIP-366's expected behavior (For compatibility reasons, in Flink 1.x, if the old configuration file flink-conf.yaml exists in the Flink conf directory, Flink will ignore the new configuration file and use the old parser to parse flink-conf.yaml as the Flink configuration. In Flink 2.x, Flink will no longer support parsing the old configuration file flink-conf.yaml).

Copy link
Contributor Author

@lincoln-lil lincoln-lil left a comment

Choose a reason for hiding this comment

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

Thanks @MartijnVisser for reviewing this!

chown -R flink:flink .; \
\
# Replace default REST/RPC endpoint bind address to use the container's network interface \
CONF_FILE="$FLINK_HOME/conf/flink-conf.yaml"; \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked both the FLINK-33297 and FLIP-366, the default configuration file has been changed to config.yaml, but the script is compatible with the old flink-conf.yaml file.
This is FLIP-366's expected behavior (For compatibility reasons, in Flink 1.x, if the old configuration file flink-conf.yaml exists in the Flink conf directory, Flink will ignore the new configuration file and use the old parser to parse flink-conf.yaml as the Flink configuration. In Flink 2.x, Flink will no longer support parsing the old configuration file flink-conf.yaml).

@lincoln-lil lincoln-lil merged commit 206aa67 into apache:master Mar 16, 2024
@lincoln-lil lincoln-lil deleted the r-master branch March 16, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants