Adding support for configuration through environment variables #12307

t0mpere · 2024-01-22T18:28:46Z

Release Notes: Adding support for Pinot configuration through ENV variables with Dynamic mapping.

From issue: #10651

export PINOT_CONTROLLER_HOST=host
export PINOT_SERVER_PROPERTY_WHATEVER=whatever_property
export ANOTHER_VARIABLE=random

-------------------- Config from CLI or Config Path ------------------------

dynamic.env.config=pinot.controller.host,pinot.server.property,other.var
pinot.controller.host=PINOT_CONTROLLER_HOST
pinot.server.property=PINOT_SERVER_PROPERTY_WHATEVER
other.var=ANOTHER_VARIABLE

----------------------- final result in memory ----------------------------

dynamic.env.config=pinot.controller.host,pinot.server.property,other.var
pinot.controller.host=host
pinot.server.property=whatever_property
other.var=random

Note:
No backward compatibility issues, ENV wasn't loaded until now.

codecov-commenter · 2024-01-22T19:07:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (21f3d28) 61.59% compared to head (c87042d) 61.74%.
Report is 55 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #12307      +/-   ##
============================================
+ Coverage     61.59%   61.74%   +0.14%     
+ Complexity     1153      207     -946     
============================================
  Files          2417     2426       +9     
  Lines        131379   132781    +1402     
  Branches      20266    20541     +275     
============================================
+ Hits          80919    81980    +1061     
- Misses        44556    44780     +224     
- Partials       5904     6021     +117

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration	`<0.01% <0.00%> (-0.01%)`	⬇️
integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration2	`0.00% <0.00%> (ø)`
java-11	`61.68% <100.00%> (+0.13%)`	⬆️
java-21	`61.62% <100.00%> (+0.16%)`	⬆️
skip-bytebuffers-false	`61.73% <100.00%> (+0.15%)`	⬆️
skip-bytebuffers-true	`61.56% <100.00%> (+0.13%)`	⬆️
temurin	`61.74% <100.00%> (+0.14%)`	⬆️
unittests	`61.73% <100.00%> (+0.14%)`	⬆️
unittests1	`46.87% <100.00%> (+0.29%)`	⬆️
unittests2	`27.75% <0.00%> (-0.08%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pinot-spi/src/main/java/org/apache/pinot/spi/env/Environment.java

pinot-spi/src/main/java/org/apache/pinot/spi/env/SystemEnvironment.java

pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java

...ller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java

pinot-minion/src/main/java/org/apache/pinot/minion/BaseMinionStarter.java

xiangfu0 · 2024-01-22T19:09:48Z

pinot-spi/src/main/java/org/apache/pinot/spi/env/PinotConfiguration.java

  }

  private static Map<String, String> relaxEnvironmentVariables(Map<String, String> environmentVariables) {
-    return environmentVariables.entrySet().stream().filter(entry -> entry.getKey().startsWith("PINOT_"))
+    return environmentVariables.entrySet().stream().filter(entry -> entry.getKey().startsWith(ENV_PREFIX))


This will break all existing changes right? Are env var like PINOT_SERVER_xxxx loaded to pinot.server.xxx?

I would suggest to keep current code rename it to legacy, so current user won't fail for using PINOT_ prefix.

Then add new code to override based on the new prefix.

At the moment (before and after my changes) <PREFIX>_SERVER_xxx is mapped to server.xxx. I've just changed the prefix to avoid confusion since pinot.server.property will be loaded from PINOT_PINOT_SERVER_PROPERTY. Considering that env wasn't loaded until now I think there's no worries about backward compatibility.

Moreover I think it would be quite hard to enforce an identical prefix to all properties. I'm for having a custom self-explanatory prefix like PINOT_ENV_. But I'm open to better ideas.

For the backward compatible perspective, I think the issue here is that for whoever currently using PINOT_CONTROLLER_HOST will see the unexpected behavior the override won't happen.

Suggest to rename existing relaxEnvVarName to legacyRelaxEnvVarName, and have your new relaxEnvVarName to override.

E.g. existing code:
PINOT_CONTROLLER_HOST should be added as controller.host;
PINOT_PINOT_CONTROLLER_HOST should be added as pinot.controller.host;

PINOT_ENV_SERVER_XXX should be added as server.xxx;
PINOT_ENV_PINOT_SERVER_XXX should be added as pinot.server.xxx;

From the ease of use perspective, actually I would love to have the direct mapping from env variable to configs, e.g.
pinot.server.xxx should be override by PINOT_SERVER_XXX.
But due to some legacy issue that not all configs are start with pinot. that's why we introduce this PINOT_ prefix.

IMO, I would love to map env var PINOT_XXX_YYY to both xxx.yyy and pinot.xxx.yyy.

So that we can cover both.

Just for xxx.yyy may override existing configs but pinot.xxx.yyy will be added if not existed.

cc: @Jackie-Jiang @snleee @siddharthteotia @chenboat would love your best practice of config override.

Implemented 1.
I'm worried that by doing 2. we could incur in conflicts with existing env vars. Prefix avoids this issues.

After catching up, number two was implemented following:

export PINOT_CONTROLLER_HOST=host export PINOT_SERVER_PROPERTY_WHATEVER=whatever_property export ANOTHER_VARIABLE=random -------------------- Config from CLI or Config Path ------------------------ dynamic.env.config=pinot.controller.host,pinot.server.property,other.var pinot.controller.host=PINOT_CONTROLLER_HOST pinot.server.property=PINOT_SERVER_PROPERTY_WHATEVER other.var=ANOTHER_VARIABLE --------------------------------------------------------------------------- config gets templated at runtime with the content of the env vars. ----------------------- final result in memory ---------------------------- dynamic.env.config=pinot.controller.host,pinot.server.property,other.var pinot.controller.host=host pinot.server.property=whatever_property other.var=random

walterddr

looks good to me. could you add more backward compatibility testing?

accidental click on approval

xiangfu0 · 2024-02-06T20:01:52Z

Please also add a section for release notes and documentation in the PR description.

xiangfu0 · 2024-02-06T20:02:12Z

Thanks for your contribution!

t0mpere · 2024-02-07T14:49:26Z

Doc added

…e#12307) * init * Added support for env vars * Revert removal of SystemEnvironment and Environment classes * Fix tests * Fix formatting * Revert unnecessary changes * Revert unnecessary changes * Formatting * Comments * Tests fix * Added back legacy prefix * Fix test * Implementing dynamic templating for env config * Fix typo

t0mpere added 6 commits January 19, 2024 15:10

init

cf63c19

update branch

cd38a41

Added support for env vars

cc721e9

Revert removal of SystemEnvironment and Environment classes

28ff0b5

Fix tests

410f179

Fix formatting

a4432b2

xiangfu0 reviewed Jan 22, 2024

View reviewed changes

t0mpere added 6 commits January 23, 2024 13:18

Revert unnecessary changes

8c03112

Revert unnecessary changes

fa3f6d7

Formatting

abd2547

Comments

a262288

Tests fix

d5505a3

Added back legacy prefix

db45ed5

t0mpere requested a review from xiangfu0 January 24, 2024 16:38

Fix test

b28eb2d

walterddr previously approved these changes Jan 31, 2024

View reviewed changes

Implementing dynamic templating for env config

2afe8ee

xiangfu0 added Configuration Config changes (addition/deletion/change in behavior) documentation labels Feb 6, 2024

xiangfu0 approved these changes Feb 6, 2024

View reviewed changes

xiangfu0 added the release-notes Referenced by PRs that need attention when compiling the next release notes label Feb 6, 2024

Fix typo

c87042d

t0mpere mentioned this pull request Feb 7, 2024

Dynamic Environment Configuration documentation pinot-contrib/pinot-docs#289

Merged

xiangfu0 merged commit 7c9bf8c into apache:master Feb 8, 2024
19 checks passed

Jackie-Jiang mentioned this pull request Feb 21, 2024

Environment Variables are not effective #10651

Closed

This was referenced May 23, 2024

do not fail on duplicate relaxed vars #13213

Closed

do not fail on duplicate relaxed vars #13214

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for configuration through environment variables #12307

Adding support for configuration through environment variables #12307

t0mpere commented Jan 22, 2024 •

edited

Loading

codecov-commenter commented Jan 22, 2024 •

edited

Loading

xiangfu0 Jan 22, 2024 •

edited

Loading

t0mpere Jan 23, 2024

t0mpere Jan 23, 2024

xiangfu0 Jan 23, 2024

t0mpere Jan 24, 2024 •

edited

Loading

t0mpere Feb 6, 2024

walterddr left a comment •

edited

Loading

xiangfu0 commented Feb 6, 2024 •

edited

Loading

xiangfu0 commented Feb 6, 2024

t0mpere commented Feb 7, 2024

Adding support for configuration through environment variables #12307

Adding support for configuration through environment variables #12307

Conversation

t0mpere commented Jan 22, 2024 • edited Loading

codecov-commenter commented Jan 22, 2024 • edited Loading

Codecov Report

xiangfu0 Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

t0mpere Jan 23, 2024

Choose a reason for hiding this comment

t0mpere Jan 23, 2024

Choose a reason for hiding this comment

xiangfu0 Jan 23, 2024

Choose a reason for hiding this comment

t0mpere Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

t0mpere Feb 6, 2024

Choose a reason for hiding this comment

walterddr left a comment • edited Loading

Choose a reason for hiding this comment

xiangfu0 commented Feb 6, 2024 • edited Loading

xiangfu0 commented Feb 6, 2024

t0mpere commented Feb 7, 2024

t0mpere commented Jan 22, 2024 •

edited

Loading

codecov-commenter commented Jan 22, 2024 •

edited

Loading

xiangfu0 Jan 22, 2024 •

edited

Loading

t0mpere Jan 24, 2024 •

edited

Loading

walterddr left a comment •

edited

Loading

xiangfu0 commented Feb 6, 2024 •

edited

Loading