[FLINK-35642] parsing the configuration file,Special handle s3 secret…

[BlackPigHe]

[FLINK-35642] parsing the configuration file,Special handle s3 secret key

[flinkbot]

CI report:

• f2f858d6497230e07c81d0c479d418b3c7fef261 Azure: FAILURE

Bot commands

The @flinkbot bot supports the following commands:

• @flinkbot run azure re-run the last Azure build

[JunRuiLee]

In regards to your pull request aiming to extend the handling of the "#" symbol, Flink has already introduced a configuration file config.yaml in version 1.19 that supports standard YAML. Please refer to this documentation for more details: https://nightlies.apache.org/flink/flink-docs-master/docs/deployment/config/#flink-configuration-file

[BlackPigHe]

In regards to your pull request aiming to extend the handling of the "#" symbol, Flink has already introduced a configuration file config.yaml in version 1.19 that supports standard YAML. Please refer to this documentation for more details: https://nightlies.apache.org/flink/flink-docs-master/docs/deployment/config/#flink-configuration-file

The description I saw was the version description for 1.19, and support for this config.yaml . Previous versions like 1.14 had to modify the code on their own

[BlackPigHe]

@JunRuiLee if version release 1.18 need maintenance,Whether this change it can be merged into the release-1.18

[JunRuiLee]

@JunRuiLee if version release 1.18 need maintenance,Whether this change it can be merged into the release-1.18

@BlackPigHe my concern is that the configuration file parsing logic is a public interface; we cannot modify it directly without community discussion. Moreover, adding logic to handle this issue without making users aware of the changes could adversely affect all users, especially as it's difficult for them to notice such silent changes. This is precisely why we need a new config file that supports the standard YAML, which would make such changes more apparent to users.

Therefore, from my perspective, I do not recommend merging these changes into version 1.18.

[BlackPigHe]

@JunRuiLee Although the change is a public module, the scope and logic of the change is relatively simple, and this can be discussed with others at the time. The main reason I want to include is that this bug is really deep and hidden, I find the cause of the problem by arthas, the developer of technical capabilities not very strong ,Locating this problem will be very difficult

[BlackPigHe]

[robobario]

Maybe an alternative to maintain compatibility would be adding a mechanism to disable the inline comment parsing.

Given it is legacy code, maybe it could be triggered by something ugly like an environment variable FLINK_DISABLE_LEGACY_YAML_COMMENTS=true. So by default the config file would be parsed exactly as it is now. If the user wanted to have '#' in any value they would be able to set this var, at the cost of having to ensure they do not use any comments in their file.

[BlackPigHe]

@robobario like this，is ok

[robobario]

Something like that, but it still likely needs discussion on the dev mailing list. I'm new too, and don't know the projects conventions so don't take my word :)

[BlackPigHe]

Something like that, but it still likely needs discussion on the dev mailing list. I'm new too, and don't know the projects conventions so don't take my word :)

liekai

[JunRuiLee]

Something like that, but it still likely needs discussion on the dev mailing list. I'm new too, and don't know the projects conventions so don't take my word :)

Yes, such changes require discussion on the developer mailing list because it's public interface.

@robobario like this，is ok

@BlackPigHe, could you attempt to use dynamic properties with the -D option to pass your configuration key-value pairs that contain the '#' symbol? IIRC, this approach would not be limited by the legacy config parser, allowing you to add your configuration correctly.

[BlackPigHe]

@JunRuiLee Added jvm parameter determination

[JunRuiLee]

@JunRuiLee Added jvm parameter determination

What I mentioned above using dynamic properties by -D could be an alternative that doesn't require changing the Flink code. You can try this approach to see if it works.

[BlackPigHe]

@JunRuiLee It is impossible to achieve the desired result without modifying the code,Because it's written in the code to split by #

[JunRuiLee]

@JunRuiLee It is impossible to achieve the desired result without modifying the code,Because it's written in the code to split by #

Please try using dynamic properties instead of a config file. What you mentioned is the logic of the config file parser.

[BlackPigHe]

@JunRuiLee It is impossible to achieve the desired result without modifying the code,Because it's written in the code to split by #

Please try using dynamic properties instead of a config file. What you mentioned is the logic of the config file parser.

@JunRuiLee In this case, the configuration will come from many aspects, Code changes are still inevitable.After all, you're loading parameters by priority

[github-actions[bot]]

This PR is being marked as stale since it has not had any activity in the last 90 days. If you would like to keep this PR alive, please leave a comment asking for a review. If the PR has merge conflicts, update it with the latest from the base branch.

If you are having difficulty finding a reviewer, please reach out to the community, contact details can be found here: https://flink.apache.org/what-is-flink/community/

If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.

[github-actions[bot]]

This PR has been closed since it has not had any activity in 120 days. If you feel like this was a mistake, or you would like to continue working on it, please feel free to re-open the PR and ask for a review.