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.

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

Do not get the environment variables twice for same property(Environment variables not identified through properties file) #14235

Merged
merged 64 commits into from
Oct 18, 2024

Conversation

deepthi912
Copy link
Collaborator

@deepthi912 deepthi912 commented Oct 15, 2024

Issue:
The environment variable is not setting right with the "dynamic.env.config" property through controller.conf file.
Reference PR: #12307

Root cause:
Ideally the dynamic variables should be added once, but BaseControllerStarter & PinotServiceManager(startRole) are both trying to apply dynamic variables and this caused an issue when setting the environment variables from the dynamic.env.config properties through config file.

Fix:

  • Added a Template flag variable to make sure that the env variables are not reloaded again after getting the value.
  • Added a info log so as to see when the variable is being set from dynamic env configs.

Test Result:

image

Changed the port to "CONTROLLER_PORT" for the controller which is "9001" from the existing config 9000:

image

Before Change: throws error when launching controller

image

After change, running controller locally on "9001" :

Screenshot 2024-10-17 at 4 05 05 PM

deepthi912 and others added 30 commits March 25, 2024 22:02
Enforce removeSegment flow with _enableDeletedKeysCompactionConsisten…
Reverse merge from master
@t0mpere
Copy link
Contributor

t0mpere commented Oct 15, 2024

If the config is applied twice then the function applyDynamicEnvConfig(List<Configuration> configurations, Map<String, String> environmentVariables) the second time gets ran will try to lookup env vars based on the values templated in the first call.
Using a templated=true|false property could prevent this when calling twice the constructor at L122 with the same configuration.

@deepthi912
Copy link
Collaborator Author

deepthi912 commented Oct 15, 2024

Yeah, it is going through already lookedup env variables again which resulted in the env variables not applied at all as second time, it will be null for that variable.

@@ -170,8 +173,15 @@ public static List<Configuration> applyDynamicEnvConfig(List<Configuration> conf
Map<String, String> environmentVariables) {
return configurations.stream().peek(configuration -> {
for (String dynamicEnvConfigVarName : configuration.getStringArray(ENV_DYNAMIC_CONFIG_KEY)) {
configuration.setProperty(dynamicEnvConfigVarName,
environmentVariables.get(configuration.getString(dynamicEnvConfigVarName)));
//if the environment variable doesn't exist or the property is already checked do not add the property
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems we are not checking whether the property already exists. Is this expected? If so, let's modify the comments

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed the description

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

Please take a look at the failed tests

@Jackie-Jiang
Copy link
Contributor

Seems the failure is caused by #14237 and not related to this PR. Will take a look and get back

@Jackie-Jiang Jackie-Jiang merged commit a26d4fc into apache:master Oct 18, 2024
18 of 21 checks passed
@Jackie-Jiang
Copy link
Contributor

This PR is causing ControllerStarterStatelessTest to fail. Please take a look

xiangfu0 pushed a commit that referenced this pull request Oct 20, 2024
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.

4 participants