-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Add interpolation to JsonConfigurator #13023
Add interpolation to JsonConfigurator #13023
Conversation
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.
Thanks for the changes, @dampcake. This is very useful!
I think this causes a regression, for example if I add the following option in MM runtime properties files for peons -
Then the MM startup will fails with
Any thoughts ? This can be reproduced by setting the above property in runtime properties file and HOSTNAME env var can be set before starting the process. |
Yeah it looks like a few configuration options that get used in the ForkingTaskRunner could use environment variables if they are passed as CLI arguments to the new Process. It's a pretty small range of options where this would have worked previously and it's not really consistent across the config system like the new option which will work for all options. There are two workarounds to get this working with the latest version.
@kfaraz @abhishekagarwal87 Any thoughts here? Should we exclude ForkingTaskRunner configs or call out is as incompatible? |
@dampcake thanks for the response but I think both options will not work -
|
@pjain1 please note it will be unescaped automatically by the StringSubstitutor if you escape it. So |
@gianm @kfaraz looks like this was missed in the release notes for 25.0.0. This is actually a breaking change for people already using other mechanism to interpolate some variables. e.g. when using the Kafka built-in |
Fixes #9617, possibly others.
Description
Add a more generic way to use environment variables, system properties, and local files as configuration values. Right now the config needs to either be a
PasswordProvider
orDynamicConfigProvider
to be able to get the value from an environment variable or other location. While those still make sense for custom implementations (eg: reading values from a secret store) or for task specs, without moving every config to one of those types it makes it hard for an operator to set configuration values from the environment.This PR uses commons-text to handle the interpolation and only supports a smaller subset of the available interpolations (see: https://commons.apache.org/proper/commons-text/apidocs/org/apache/commons/text/lookup/StringLookupFactory.html). Upstream they are already looking to disable some of the current defaults as they execute code (javascript) or reach out to external machines (see: apache/commons-text#341) so I started with a much smaller list that should unblock most issues people have.
Key changed/added classes in this PR
JsonConfigurator
This PR has: