You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/11/17 03:33:36 UTC

[GitHub] [incubator-pinot] fx19880617 opened a new pull request #6271: Adding config utils to apply environment variables and apply it to table config

fx19880617 opened a new pull request #6271:
URL: https://github.com/apache/incubator-pinot/pull/6271


   ## Description
   Support using environment variables in Pinot table configs with utils to apply the env variables.
   
   This feature allows users to hide the secret(AWS credential) or config cluster based configs (Kafka broker list) with a common table config template.
   
   Sample Table config:
   ```
   {
     "tableName": "pullRequestMergedEvents",
     "tableType": "REALTIME",
     "segmentsConfig": {
       "timeColumnName": "mergedTimeMillis",
       "timeType": "MILLISECONDS",
       "retentionTimeUnit": "DAYS",
       "retentionTimeValue": "60",
       "schemaName": "pullRequestMergedEvents",
       "replication": "1",
       "replicasPerPartition": "1"
     },
     "tenants": {},
     "tableIndexConfig": {
       "loadMode": "MMAP",
       "invertedIndexColumns": [
         "organization",
         "repo"
       ],
       "streamConfigs": {
         "streamType": "kafka",
         "stream.kafka.consumer.type": "simple",
         "stream.kafka.topic.name": "pullRequestMergedEvents",
         "stream.kafka.decoder.class.name": "org.apache.pinot.plugin.stream.kafka.KafkaJSONMessageDecoder",
         "stream.kafka.consumer.factory.class.name": "org.apache.pinot.plugin.stream.kafka20.KafkaConsumerFactory",
         "stream.kafka.zk.broker.url": "${AWS_MSK_ZK}",
         "stream.kafka.broker.list": "${AWS_MSK_KAFKA}",
         "realtime.segment.flush.threshold.time": "12h",
         "realtime.segment.flush.threshold.size": "100000",
         "stream.kafka.consumer.prop.auto.offset.reset": "smallest"
       }
     },
     "metadata": {
       "customConfigs": {}
     }
   }
   ```


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 commented on pull request #6271: Adding config utils to apply environment variables and apply it to table config

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on pull request #6271:
URL: https://github.com/apache/incubator-pinot/pull/6271#issuecomment-729150917


   > Thanks for adding this feature, very useful.
   > 
   > Shall we extend it a bit to support default value when env var not set? like `${VAR:default_value}`
   
   will add this 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] yupeng9 commented on a change in pull request #6271: Adding config utils to apply environment variables and apply it to table config

Posted by GitBox <gi...@apache.org>.
yupeng9 commented on a change in pull request #6271:
URL: https://github.com/apache/incubator-pinot/pull/6271#discussion_r525389405



##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/ConfigUtils.java
##########
@@ -0,0 +1,90 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.config;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.node.ArrayNode;
+import com.fasterxml.jackson.databind.node.JsonNodeFactory;
+import com.fasterxml.jackson.databind.node.JsonNodeType;
+import java.io.IOException;
+import java.util.Iterator;
+import java.util.Map;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+public class ConfigUtils {
+  private static final Map<String, String> ENVIRONMENT_VARIABLES = System.getenv();
+
+  /**
+   * Apply environment variables to any given BaseJsonConfig.
+   *
+   * @return Config with environment variable applied.
+   */
+  public static <T extends BaseJsonConfig> T applyConfigWithEnvVariables(T config) {
+    JsonNode jsonNode;
+    try {
+      jsonNode = applyConfigWithEnvVariables(config.toJsonNode());
+    } catch (RuntimeException e) {
+      throw new RuntimeException(String
+          .format("Unable to apply environment variables on json config class [%s].", config.getClass().getName()), e);
+    }
+    try {
+      return (T) JsonUtils.jsonNodeToObject(jsonNode, config.getClass());
+    } catch (IOException e) {
+      throw new RuntimeException(String
+          .format("Unable to read JsonConfig to class [%s] after applying environment variables, jsonConfig is: '%s'.",
+              config.getClass().getName(), jsonNode.toString()), e);
+    }
+  }
+
+  private static JsonNode applyConfigWithEnvVariables(JsonNode jsonNode) {
+    final JsonNodeType nodeType = jsonNode.getNodeType();
+    switch (nodeType) {
+      case OBJECT:
+        if (jsonNode.size() > 0) {
+          Iterator<Map.Entry<String, JsonNode>> iterator = jsonNode.fields();
+          while (iterator.hasNext()) {
+            final Map.Entry<String, JsonNode> next = iterator.next();
+            next.setValue(applyConfigWithEnvVariables(next.getValue()));
+          }
+        }
+        break;
+      case ARRAY:
+        if (jsonNode.isArray()) {
+          ArrayNode arrayNode = (ArrayNode) jsonNode;
+          for (int i = 0; i < arrayNode.size(); i++) {
+            JsonNode arrayElement = arrayNode.get(i);
+            arrayNode.set(i, applyConfigWithEnvVariables(arrayElement));
+          }
+        }
+        break;
+      case STRING:
+        final String field = jsonNode.asText();
+        if (field.startsWith("${") && field.endsWith("}")) {
+          final String envVarKey = field.substring(2, field.length() - 1);
+          if (ENVIRONMENT_VARIABLES.containsKey(envVarKey)) {

Review comment:
       shall it be case insenstive or require all caps? 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 commented on pull request #6271: Adding config utils to apply environment variables and apply it to table config

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on pull request #6271:
URL: https://github.com/apache/incubator-pinot/pull/6271#issuecomment-729183112


   @Aka-shi ^^


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 commented on pull request #6271: Adding config utils to apply environment variables and apply it to table config

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on pull request #6271:
URL: https://github.com/apache/incubator-pinot/pull/6271#issuecomment-730126453


   > > > This is going to create some issues in our validations that were recently added. For example - we look for the index columns in the schema, but user could have put an env variable. While this exact example is unlikely, there could be other legit cases. We'll have to handle this in the validations somehow.
   > > 
   > > 
   > > I feel those validations are fine as we may not want users to create a table and let servers read those configs from env variables.
   > 
   > I didn't follow how it would be fine. This will result in users not being able to create a table with env variables, or worse, table will get created with incorrect config. In your example:
   > "realtime.segment.flush.threshold.time": "${REALTIME_SEGMENT_FLUSH_THRESHOD_TIME:12h}"
   > This will fail validation, and then we'll end up setting a default value of 5m.
   
   In that case, we should fail the table creation request right?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 commented on a change in pull request #6271: Adding config utils to apply environment variables and apply it to table config

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6271:
URL: https://github.com/apache/incubator-pinot/pull/6271#discussion_r525429748



##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/ConfigUtils.java
##########
@@ -0,0 +1,90 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.config;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.node.ArrayNode;
+import com.fasterxml.jackson.databind.node.JsonNodeFactory;
+import com.fasterxml.jackson.databind.node.JsonNodeType;
+import java.io.IOException;
+import java.util.Iterator;
+import java.util.Map;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+public class ConfigUtils {
+  private static final Map<String, String> ENVIRONMENT_VARIABLES = System.getenv();
+
+  /**
+   * Apply environment variables to any given BaseJsonConfig.
+   *
+   * @return Config with environment variable applied.
+   */
+  public static <T extends BaseJsonConfig> T applyConfigWithEnvVariables(T config) {
+    JsonNode jsonNode;
+    try {
+      jsonNode = applyConfigWithEnvVariables(config.toJsonNode());
+    } catch (RuntimeException e) {
+      throw new RuntimeException(String
+          .format("Unable to apply environment variables on json config class [%s].", config.getClass().getName()), e);
+    }
+    try {
+      return (T) JsonUtils.jsonNodeToObject(jsonNode, config.getClass());
+    } catch (IOException e) {
+      throw new RuntimeException(String
+          .format("Unable to read JsonConfig to class [%s] after applying environment variables, jsonConfig is: '%s'.",
+              config.getClass().getName(), jsonNode.toString()), e);
+    }
+  }
+
+  private static JsonNode applyConfigWithEnvVariables(JsonNode jsonNode) {
+    final JsonNodeType nodeType = jsonNode.getNodeType();
+    switch (nodeType) {
+      case OBJECT:
+        if (jsonNode.size() > 0) {
+          Iterator<Map.Entry<String, JsonNode>> iterator = jsonNode.fields();
+          while (iterator.hasNext()) {
+            final Map.Entry<String, JsonNode> next = iterator.next();
+            next.setValue(applyConfigWithEnvVariables(next.getValue()));
+          }
+        }
+        break;
+      case ARRAY:
+        if (jsonNode.isArray()) {
+          ArrayNode arrayNode = (ArrayNode) jsonNode;
+          for (int i = 0; i < arrayNode.size(); i++) {
+            JsonNode arrayElement = arrayNode.get(i);
+            arrayNode.set(i, applyConfigWithEnvVariables(arrayElement));
+          }
+        }
+        break;
+      case STRING:
+        final String field = jsonNode.asText();
+        if (field.startsWith("${") && field.endsWith("}")) {
+          final String envVarKey = field.substring(2, field.length() - 1);
+          if (ENVIRONMENT_VARIABLES.containsKey(envVarKey)) {

Review comment:
       The names of environment variables are case-sensitive; by convention they are uppercase, so technically you can define env var: `a` and `A` with different values.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6271: Adding config utils to apply environment variables and apply it to table config

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6271:
URL: https://github.com/apache/incubator-pinot/pull/6271#issuecomment-728675017


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6271?src=pr&el=h1) Report
   > Merging [#6271](https://codecov.io/gh/apache/incubator-pinot/pull/6271?src=pr&el=desc) (21dde94) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `2.33%`.
   > The diff coverage is `45.12%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6271/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/6271?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #6271      +/-   ##
   ==========================================
   - Coverage   66.44%   64.11%   -2.34%     
   ==========================================
     Files        1075     1244     +169     
     Lines       54773    58984    +4211     
     Branches     8168     8746     +578     
   ==========================================
   + Hits        36396    37815    +1419     
   - Misses      15700    18407    +2707     
   - Partials     2677     2762      +85     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | unittests | `64.11% <45.12%> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6271?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
   | [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
   | [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
   | [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <0.00%> (+9.52%)` | :arrow_up: |
   | [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <0.00%> (-13.29%)` | :arrow_down: |
   | [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `10.90% <0.00%> (-51.10%)` | :arrow_down: |
   | [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `73.80% <ø> (+0.63%)` | :arrow_up: |
   | [.../apache/pinot/common/exception/QueryException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZXhjZXB0aW9uL1F1ZXJ5RXhjZXB0aW9uLmphdmE=) | `90.27% <ø> (+5.55%)` | :arrow_up: |
   | [...pinot/common/function/AggregationFunctionType.java](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vQWdncmVnYXRpb25GdW5jdGlvblR5cGUuamF2YQ==) | `98.27% <ø> (-1.73%)` | :arrow_down: |
   | ... and [1083 more](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6271?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6271?src=pr&el=footer). Last update [5a53fbe...21dde94](https://codecov.io/gh/apache/incubator-pinot/pull/6271?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 edited a comment on pull request #6271: Adding config utils to apply environment variables and apply it to table config

Posted by GitBox <gi...@apache.org>.
fx19880617 edited a comment on pull request #6271:
URL: https://github.com/apache/incubator-pinot/pull/6271#issuecomment-729150917


   > Thanks for adding this feature, very useful.
   > 
   > Shall we extend it a bit to support default value when env var not set? like `${VAR:default_value}`
   
   Added.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 commented on pull request #6271: Adding config utils to apply environment variables and apply it to table config

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on pull request #6271:
URL: https://github.com/apache/incubator-pinot/pull/6271#issuecomment-729989881


   > @fx19880617 While supporting env variable seems good for local deployments, I am unsure how that works for real prod deployments. How do we propagate the credentials to all participants? Or do they query the controller each time?
   > 
   > I am approving the env variable support change. But should we explore using Docker secrets instead, for storing credentials?
   
   yes, typically for prod env, those credentials are local on the machine, e.g. in k8s, we can create a secret and set it as environment variables when we start the container.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] yupeng9 commented on pull request #6271: Adding config utils to apply environment variables and apply it to table config

Posted by GitBox <gi...@apache.org>.
yupeng9 commented on pull request #6271:
URL: https://github.com/apache/incubator-pinot/pull/6271#issuecomment-729114058


   Thanks for adding this feature, very useful.
   
   Shall we extend it a bit to support default value when env var not set? like `${VAR:default_value}`


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] npawar commented on pull request #6271: Adding config utils to apply environment variables and apply it to table config

Posted by GitBox <gi...@apache.org>.
npawar commented on pull request #6271:
URL: https://github.com/apache/incubator-pinot/pull/6271#issuecomment-730043495


   > > This is going to create some issues in our validations that were recently added. For example - we look for the index columns in the schema, but user could have put an env variable. While this exact example is unlikely, there could be other legit cases. We'll have to handle this in the validations somehow.
   > 
   > I feel those validations are fine as we may not want users to create a table and let servers read those configs from env variables.
   
   I didn't follow how it would be fine. This will result in users not being able to create a table with env variables, or worse, table will get created with incorrect config. In your example: 
   "realtime.segment.flush.threshold.time": "${REALTIME_SEGMENT_FLUSH_THRESHOD_TIME:12h}"
   This will fail validation, and then we'll end up setting a default value of 5m.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 edited a comment on pull request #6271: Adding config utils to apply environment variables and apply it to table config

Posted by GitBox <gi...@apache.org>.
fx19880617 edited a comment on pull request #6271:
URL: https://github.com/apache/incubator-pinot/pull/6271#issuecomment-729989881


   > @fx19880617 While supporting env variable seems good for local deployments, I am unsure how that works for real prod deployments. How do we propagate the credentials to all participants? Or do they query the controller each time?
   > 
   > I am approving the env variable support change. But should we explore using Docker secrets instead, for storing credentials?
   
   yes, typically for prod env, those credentials are local on the machine, and we can read it as env variable.
   For deployment on k8s, we can create a secret and set it as environment variables when we start the container.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] yupeng9 commented on a change in pull request #6271: Adding config utils to apply environment variables and apply it to table config

Posted by GitBox <gi...@apache.org>.
yupeng9 commented on a change in pull request #6271:
URL: https://github.com/apache/incubator-pinot/pull/6271#discussion_r525543517



##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/ConfigUtils.java
##########
@@ -0,0 +1,90 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.config;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.node.ArrayNode;
+import com.fasterxml.jackson.databind.node.JsonNodeFactory;
+import com.fasterxml.jackson.databind.node.JsonNodeType;
+import java.io.IOException;
+import java.util.Iterator;
+import java.util.Map;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+public class ConfigUtils {
+  private static final Map<String, String> ENVIRONMENT_VARIABLES = System.getenv();
+
+  /**
+   * Apply environment variables to any given BaseJsonConfig.
+   *
+   * @return Config with environment variable applied.
+   */
+  public static <T extends BaseJsonConfig> T applyConfigWithEnvVariables(T config) {
+    JsonNode jsonNode;
+    try {
+      jsonNode = applyConfigWithEnvVariables(config.toJsonNode());
+    } catch (RuntimeException e) {
+      throw new RuntimeException(String
+          .format("Unable to apply environment variables on json config class [%s].", config.getClass().getName()), e);
+    }
+    try {
+      return (T) JsonUtils.jsonNodeToObject(jsonNode, config.getClass());
+    } catch (IOException e) {
+      throw new RuntimeException(String
+          .format("Unable to read JsonConfig to class [%s] after applying environment variables, jsonConfig is: '%s'.",
+              config.getClass().getName(), jsonNode.toString()), e);
+    }
+  }
+
+  private static JsonNode applyConfigWithEnvVariables(JsonNode jsonNode) {
+    final JsonNodeType nodeType = jsonNode.getNodeType();
+    switch (nodeType) {
+      case OBJECT:
+        if (jsonNode.size() > 0) {
+          Iterator<Map.Entry<String, JsonNode>> iterator = jsonNode.fields();
+          while (iterator.hasNext()) {
+            final Map.Entry<String, JsonNode> next = iterator.next();
+            next.setValue(applyConfigWithEnvVariables(next.getValue()));
+          }
+        }
+        break;
+      case ARRAY:
+        if (jsonNode.isArray()) {
+          ArrayNode arrayNode = (ArrayNode) jsonNode;
+          for (int i = 0; i < arrayNode.size(); i++) {
+            JsonNode arrayElement = arrayNode.get(i);
+            arrayNode.set(i, applyConfigWithEnvVariables(arrayElement));
+          }
+        }
+        break;
+      case STRING:
+        final String field = jsonNode.asText();
+        if (field.startsWith("${") && field.endsWith("}")) {
+          final String envVarKey = field.substring(2, field.length() - 1);
+          if (ENVIRONMENT_VARIABLES.containsKey(envVarKey)) {

Review comment:
       yup, it makes sense to follow unix convention




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 merged pull request #6271: Adding config utils to apply environment variables and apply it to table config

Posted by GitBox <gi...@apache.org>.
fx19880617 merged pull request #6271:
URL: https://github.com/apache/incubator-pinot/pull/6271


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 edited a comment on pull request #6271: Adding config utils to apply environment variables and apply it to table config

Posted by GitBox <gi...@apache.org>.
fx19880617 edited a comment on pull request #6271:
URL: https://github.com/apache/incubator-pinot/pull/6271#issuecomment-729989881


   > @fx19880617 While supporting env variable seems good for local deployments, I am unsure how that works for real prod deployments. How do we propagate the credentials to all participants? Or do they query the controller each time?
   > 
   > I am approving the env variable support change. But should we explore using Docker secrets instead, for storing credentials?
   
   yes, typically for prod env, those credentials are either local on the machine, and we can read it as env variable.
   For deployment on k8s, we can create a secret and set it as environment variables when we start the container.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 commented on pull request #6271: Adding config utils to apply environment variables and apply it to table config

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on pull request #6271:
URL: https://github.com/apache/incubator-pinot/pull/6271#issuecomment-730040623


   > This is going to create some issues in our validations that were recently added. For example - we look for the index columns in the schema, but user could have put an env variable. While this exact example is unlikely, there could be other legit cases. We'll have to handle this in the validations somehow.
   
   I feel those validations are fine as we may not want users to create a table and let servers read those configs from env variables.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] npawar commented on pull request #6271: Adding config utils to apply environment variables and apply it to table config

Posted by GitBox <gi...@apache.org>.
npawar commented on pull request #6271:
URL: https://github.com/apache/incubator-pinot/pull/6271#issuecomment-730030842


   This is going to create some issues in our validations that were recently added. For example - we look for the index columns in the schema, but user could have put an env variable. While this exact example is unlikely, there could be other legit cases. We'll have to handle this in the validations somehow.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-io commented on pull request #6271: Adding config utils to apply environment variables and apply it to table config

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6271:
URL: https://github.com/apache/incubator-pinot/pull/6271#issuecomment-728675017


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6271?src=pr&el=h1) Report
   > Merging [#6271](https://codecov.io/gh/apache/incubator-pinot/pull/6271?src=pr&el=desc) (4b44e3d) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `2.32%`.
   > The diff coverage is `45.12%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6271/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/6271?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #6271      +/-   ##
   ==========================================
   - Coverage   66.44%   64.12%   -2.33%     
   ==========================================
     Files        1075     1244     +169     
     Lines       54773    58984    +4211     
     Branches     8168     8746     +578     
   ==========================================
   + Hits        36396    37825    +1429     
   - Misses      15700    18399    +2699     
   - Partials     2677     2760      +83     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | unittests | `64.12% <45.12%> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6271?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
   | [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
   | [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
   | [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <0.00%> (+9.52%)` | :arrow_up: |
   | [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <0.00%> (-13.29%)` | :arrow_down: |
   | [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `10.90% <0.00%> (-51.10%)` | :arrow_down: |
   | [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `73.80% <ø> (+0.63%)` | :arrow_up: |
   | [.../apache/pinot/common/exception/QueryException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZXhjZXB0aW9uL1F1ZXJ5RXhjZXB0aW9uLmphdmE=) | `90.27% <ø> (+5.55%)` | :arrow_up: |
   | [...pinot/common/function/AggregationFunctionType.java](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vQWdncmVnYXRpb25GdW5jdGlvblR5cGUuamF2YQ==) | `98.27% <ø> (-1.73%)` | :arrow_down: |
   | ... and [1082 more](https://codecov.io/gh/apache/incubator-pinot/pull/6271/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6271?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6271?src=pr&el=footer). Last update [5a53fbe...21dde94](https://codecov.io/gh/apache/incubator-pinot/pull/6271?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org