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 2021/06/03 22:28:42 UTC

[GitHub] [incubator-pinot] desaijay230592 opened a new pull request #7019: Resolve bug in HelixServerStarter for populating environment property…

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


   … map field and rename timeout variables to follow timeunit convention
   
   ## Description
   There was a bug in the HelixServerStarter code which would prevent us to update zookeeper node's mapFields with correct environment properties. Secondly, changed the AzureEnvironmentProvider variable names to include time units as suffix for better readability and debuggability. 
   
   Previous Wrong Syntax:
   ```
   Map<String, String> existingEnvironmentConfigsMap = instanceConfig.getRecord().getMapField(
             CommonConstants.ENVIRONMENT_IDENTIFIER);
   if (existingEnvironmentConfigsMap != null && !existingEnvironmentConfigsMap.equals(environmentProperties)) {
     do Something......
   }
   ```
   The above check would prevent us to add values to mapFields for environment key if it not already present.
   
   Corrected Syntax:
   ```
   Map<String, String> existingEnvironmentConfigsMap = instanceConfig.getRecord().getMapField(
             CommonConstants.ENVIRONMENT_IDENTIFIER);
   if (existingEnvironmentConfigsMap == null || !existingEnvironmentConfigsMap.equals(environmentProperties)) { 
     do Something......
   }
   
   ```
   The corrected check will resolve the bug shown in the faulty syntax and will update mapFields for environment key when it is not present in the map as well as when the value set has a mismatch.
   
   ## Upgrade Notes
   Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
   * [ ] Yes (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
   
   Does this PR fix a zero-downtime upgrade introduced earlier?
   * [ ] Yes (Please label this as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
   
   Does this PR otherwise need attention when creating release notes? Things to consider:
   - New configuration options
   
   * [ ] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
   ## Release Notes
   <!-- If you have tagged this as either backward-incompat or release-notes,
   you MUST add text here that you would like to see appear in release notes of the
   next release. -->
   
   <!-- If you have a series of commits adding or enabling a feature, then
   add this section only in final commit that marks the feature completed.
   Refer to earlier release notes to see examples of text.
   -->
   ## Documentation
   
   ![Screen Shot 2021-06-03 at 3 24 06 PM](https://user-images.githubusercontent.com/8770850/120719873-e3ff9280-c47f-11eb-90a6-562e88f875cf.png)
   


-- 
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] mcvsubbu merged pull request #7019: Resolve bug in HelixServerStarter for populating environment property…

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


   


-- 
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-commenter edited a comment on pull request #7019: Resolve bug in HelixServerStarter for populating environment property…

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


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#7019](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d35c45e) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/06e7ac6e9e472dc9f96eec6733c02cbc3e6b0b15?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (06e7ac6) will **decrease** coverage by `0.08%`.
   > The diff coverage is `0.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/7019/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #7019      +/-   ##
   ============================================
   - Coverage     73.27%   73.19%   -0.09%     
     Complexity       12       12              
   ============================================
     Files          1452     1452              
     Lines         71960    71960              
     Branches      10417    10417              
   ============================================
   - Hits          52731    52671      -60     
   - Misses        15702    15761      +59     
   - Partials       3527     3528       +1     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration | `41.81% <0.00%> (-0.09%)` | :arrow_down: |
   | unittests | `65.39% <0.00%> (-0.03%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...pinot/server/starter/helix/HelixServerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9IZWxpeFNlcnZlclN0YXJ0ZXIuamF2YQ==) | `49.03% <0.00%> (ø)` | |
   | [...ot/segment/local/customobject/MinMaxRangePair.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9jdXN0b21vYmplY3QvTWluTWF4UmFuZ2VQYWlyLmphdmE=) | `75.86% <0.00%> (-24.14%)` | :arrow_down: |
   | [.../helix/core/minion/MinionInstancesCleanupTask.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9NaW5pb25JbnN0YW5jZXNDbGVhbnVwVGFzay5qYXZh) | `57.14% <0.00%> (-14.29%)` | :arrow_down: |
   | [...impl/dictionary/DoubleOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRG91YmxlT25IZWFwTXV0YWJsZURpY3Rpb25hcnkuamF2YQ==) | `46.98% <0.00%> (-13.26%)` | :arrow_down: |
   | [...e/impl/dictionary/LongOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvTG9uZ09uSGVhcE11dGFibGVEaWN0aW9uYXJ5LmphdmE=) | `63.85% <0.00%> (-12.05%)` | :arrow_down: |
   | [.../impl/dictionary/LongOffHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvTG9uZ09mZkhlYXBNdXRhYmxlRGljdGlvbmFyeS5qYXZh) | `67.02% <0.00%> (-5.32%)` | :arrow_down: |
   | [...core/query/executor/ServerQueryExecutorV1Impl.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9leGVjdXRvci9TZXJ2ZXJRdWVyeUV4ZWN1dG9yVjFJbXBsLmphdmE=) | `79.45% <0.00%> (-4.87%)` | :arrow_down: |
   | [...cal/startree/v2/builder/BaseSingleTreeBuilder.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS92Mi9idWlsZGVyL0Jhc2VTaW5nbGVUcmVlQnVpbGRlci5qYXZh) | `88.39% <0.00%> (-4.47%)` | :arrow_down: |
   | [...pinot/core/query/request/context/TimerContext.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZXF1ZXN0L2NvbnRleHQvVGltZXJDb250ZXh0LmphdmE=) | `91.66% <0.00%> (-4.17%)` | :arrow_down: |
   | [...ot/segment/local/startree/OffHeapStarTreeNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS9PZmZIZWFwU3RhclRyZWVOb2RlLmphdmE=) | `81.48% <0.00%> (-3.71%)` | :arrow_down: |
   | ... and [17 more](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [06e7ac6...d35c45e](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] desaijay230592 commented on a change in pull request #7019: Resolve bug in HelixServerStarter for populating environment property…

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



##########
File path: pinot-plugins/pinot-environment/pinot-azure/src/main/java/org/apache/pinot/plugin/provider/AzureEnvironmentProvider.java
##########
@@ -40,21 +40,17 @@
 import org.apache.http.util.EntityUtils;
 import org.apache.pinot.spi.env.PinotConfiguration;
 import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 
 /**
  * Azure Environment Provider used to retrieve azure cloud specific instance configuration.
  */
 public class AzureEnvironmentProvider implements PinotEnvironmentProvider {
 
-  private static final Logger LOGGER = LoggerFactory.getLogger(AzureEnvironmentProvider.class);
-
   protected static final String MAX_RETRY = "maxRetry";
   protected static final String IMDS_ENDPOINT = "imdsEndpoint";
-  protected static final String CONNECTION_TIMEOUT = "connectionTimeout";
-  protected static final String REQUEST_TIMEOUT = "requestTimeout";
+  protected static final String CONNECTION_TIMEOUT_MILLIS = "connectionTimeoutMillis";
+  protected static final String REQUEST_TIMEOUT_MILLIS = "requestTimeoutMillis";

Review comment:
       @Jackie-Jiang  Saw some references where they are using MILLIS as suffix too like in StreamConfig.java.
   Let me know if its fine to keep MILLIS as suffix.




-- 
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] mcvsubbu commented on a change in pull request #7019: Resolve bug in HelixServerStarter for populating environment property…

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



##########
File path: pinot-plugins/pinot-environment/pinot-azure/src/main/java/org/apache/pinot/plugin/provider/AzureEnvironmentProvider.java
##########
@@ -40,21 +40,17 @@
 import org.apache.http.util.EntityUtils;
 import org.apache.pinot.spi.env.PinotConfiguration;
 import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 
 /**
  * Azure Environment Provider used to retrieve azure cloud specific instance configuration.
  */
 public class AzureEnvironmentProvider implements PinotEnvironmentProvider {
 
-  private static final Logger LOGGER = LoggerFactory.getLogger(AzureEnvironmentProvider.class);
-
   protected static final String MAX_RETRY = "maxRetry";
   protected static final String IMDS_ENDPOINT = "imdsEndpoint";
-  protected static final String CONNECTION_TIMEOUT = "connectionTimeout";
-  protected static final String REQUEST_TIMEOUT = "requestTimeout";
+  protected static final String CONNECTION_TIMEOUT_MILLIS = "connectionTimeoutMillis";
+  protected static final String REQUEST_TIMEOUT_MILLIS = "requestTimeoutMillis";

Review comment:
       Agreed. We don't seem to have a convention either way, so I am fine having this as it is. @Jackie-Jiang let me know if we are moving towards a convention that I am not aware of.




-- 
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] Jackie-Jiang commented on a change in pull request #7019: Resolve bug in HelixServerStarter for populating environment property…

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7019:
URL: https://github.com/apache/incubator-pinot/pull/7019#discussion_r645209309



##########
File path: pinot-plugins/pinot-environment/pinot-azure/src/main/java/org/apache/pinot/plugin/provider/AzureEnvironmentProvider.java
##########
@@ -40,21 +40,17 @@
 import org.apache.http.util.EntityUtils;
 import org.apache.pinot.spi.env.PinotConfiguration;
 import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 
 /**
  * Azure Environment Provider used to retrieve azure cloud specific instance configuration.
  */
 public class AzureEnvironmentProvider implements PinotEnvironmentProvider {
 
-  private static final Logger LOGGER = LoggerFactory.getLogger(AzureEnvironmentProvider.class);
-
   protected static final String MAX_RETRY = "maxRetry";
   protected static final String IMDS_ENDPOINT = "imdsEndpoint";
-  protected static final String CONNECTION_TIMEOUT = "connectionTimeout";
-  protected static final String REQUEST_TIMEOUT = "requestTimeout";
+  protected static final String CONNECTION_TIMEOUT_MILLIS = "connectionTimeoutMillis";
+  protected static final String REQUEST_TIMEOUT_MILLIS = "requestTimeoutMillis";

Review comment:
       Seems we have both configs, so either way is fine to me




-- 
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-commenter commented on pull request #7019: Resolve bug in HelixServerStarter for populating environment property…

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


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#7019](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d35c45e) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/06e7ac6e9e472dc9f96eec6733c02cbc3e6b0b15?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (06e7ac6) will **decrease** coverage by `7.88%`.
   > The diff coverage is `0.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/7019/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #7019      +/-   ##
   ============================================
   - Coverage     73.27%   65.39%   -7.89%     
     Complexity       12       12              
   ============================================
     Files          1452     1452              
     Lines         71960    71960              
     Branches      10417    10417              
   ============================================
   - Hits          52731    47060    -5671     
   - Misses        15702    21504    +5802     
   + Partials       3527     3396     -131     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration | `?` | |
   | unittests | `65.39% <0.00%> (-0.03%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...pinot/server/starter/helix/HelixServerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9IZWxpeFNlcnZlclN0YXJ0ZXIuamF2YQ==) | `0.00% <0.00%> (-49.04%)` | :arrow_down: |
   | [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...t/core/startree/plan/StarTreeDocIdSetPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlRG9jSWRTZXRQbGFuTm9kZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../core/startree/plan/StarTreeTransformPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlVHJhbnNmb3JtUGxhbk5vZGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...core/startree/plan/StarTreeProjectionPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlUHJvamVjdGlvblBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...t/minion/executor/MinionTaskZkMetadataManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhlY3V0b3IvTWluaW9uVGFza1prTWV0YWRhdGFNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [338 more](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [06e7ac6...d35c45e](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] mcvsubbu commented on a change in pull request #7019: Resolve bug in HelixServerStarter for populating environment property…

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



##########
File path: pinot-plugins/pinot-environment/pinot-azure/src/main/java/org/apache/pinot/plugin/provider/AzureEnvironmentProvider.java
##########
@@ -40,21 +40,17 @@
 import org.apache.http.util.EntityUtils;
 import org.apache.pinot.spi.env.PinotConfiguration;
 import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 
 /**
  * Azure Environment Provider used to retrieve azure cloud specific instance configuration.
  */
 public class AzureEnvironmentProvider implements PinotEnvironmentProvider {
 
-  private static final Logger LOGGER = LoggerFactory.getLogger(AzureEnvironmentProvider.class);
-
   protected static final String MAX_RETRY = "maxRetry";
   protected static final String IMDS_ENDPOINT = "imdsEndpoint";
-  protected static final String CONNECTION_TIMEOUT = "connectionTimeout";
-  protected static final String REQUEST_TIMEOUT = "requestTimeout";
+  protected static final String CONNECTION_TIMEOUT_MILLIS = "connectionTimeoutMillis";
+  protected static final String REQUEST_TIMEOUT_MILLIS = "requestTimeoutMillis";

Review comment:
       Agreed. We don't seem to have a convention either way, so I am fine having this as it is. @Jackie-Jiang let me know if we are moving towards a convention that I am not aware of.




-- 
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] Jackie-Jiang commented on a change in pull request #7019: Resolve bug in HelixServerStarter for populating environment property…

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7019:
URL: https://github.com/apache/incubator-pinot/pull/7019#discussion_r645195262



##########
File path: pinot-plugins/pinot-environment/pinot-azure/src/main/java/org/apache/pinot/plugin/provider/AzureEnvironmentProvider.java
##########
@@ -40,21 +40,17 @@
 import org.apache.http.util.EntityUtils;
 import org.apache.pinot.spi.env.PinotConfiguration;
 import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 
 /**
  * Azure Environment Provider used to retrieve azure cloud specific instance configuration.
  */
 public class AzureEnvironmentProvider implements PinotEnvironmentProvider {
 
-  private static final Logger LOGGER = LoggerFactory.getLogger(AzureEnvironmentProvider.class);
-
   protected static final String MAX_RETRY = "maxRetry";
   protected static final String IMDS_ENDPOINT = "imdsEndpoint";
-  protected static final String CONNECTION_TIMEOUT = "connectionTimeout";
-  protected static final String REQUEST_TIMEOUT = "requestTimeout";
+  protected static final String CONNECTION_TIMEOUT_MILLIS = "connectionTimeoutMillis";
+  protected static final String REQUEST_TIMEOUT_MILLIS = "requestTimeoutMillis";

Review comment:
       To be consistent with other config keys in the code base
   ```suggestion
     protected static final String CONNECTION_TIMEOUT_MS = "connectionTimeoutMs";
     protected static final String REQUEST_TIMEOUT_MS = "requestTimeoutMs";
   ```

##########
File path: pinot-plugins/pinot-environment/pinot-azure/src/main/java/org/apache/pinot/plugin/provider/AzureEnvironmentProvider.java
##########
@@ -40,21 +40,17 @@
 import org.apache.http.util.EntityUtils;
 import org.apache.pinot.spi.env.PinotConfiguration;
 import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 
 /**
  * Azure Environment Provider used to retrieve azure cloud specific instance configuration.
  */
 public class AzureEnvironmentProvider implements PinotEnvironmentProvider {
 
-  private static final Logger LOGGER = LoggerFactory.getLogger(AzureEnvironmentProvider.class);
-
   protected static final String MAX_RETRY = "maxRetry";
   protected static final String IMDS_ENDPOINT = "imdsEndpoint";
-  protected static final String CONNECTION_TIMEOUT = "connectionTimeout";
-  protected static final String REQUEST_TIMEOUT = "requestTimeout";
+  protected static final String CONNECTION_TIMEOUT_MILLIS = "connectionTimeoutMillis";
+  protected static final String REQUEST_TIMEOUT_MILLIS = "requestTimeoutMillis";

Review comment:
       Seems we have both configs, so either way is fine to me




-- 
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] mcvsubbu merged pull request #7019: Resolve bug in HelixServerStarter for populating environment property…

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


   


-- 
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-commenter edited a comment on pull request #7019: Resolve bug in HelixServerStarter for populating environment property…

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


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#7019](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d35c45e) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/06e7ac6e9e472dc9f96eec6733c02cbc3e6b0b15?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (06e7ac6) will **decrease** coverage by `0.08%`.
   > The diff coverage is `0.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/7019/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #7019      +/-   ##
   ============================================
   - Coverage     73.27%   73.19%   -0.09%     
     Complexity       12       12              
   ============================================
     Files          1452     1452              
     Lines         71960    71960              
     Branches      10417    10417              
   ============================================
   - Hits          52731    52671      -60     
   - Misses        15702    15761      +59     
   - Partials       3527     3528       +1     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration | `41.81% <0.00%> (-0.09%)` | :arrow_down: |
   | unittests | `65.39% <0.00%> (-0.03%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...pinot/server/starter/helix/HelixServerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9IZWxpeFNlcnZlclN0YXJ0ZXIuamF2YQ==) | `49.03% <0.00%> (ø)` | |
   | [...ot/segment/local/customobject/MinMaxRangePair.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9jdXN0b21vYmplY3QvTWluTWF4UmFuZ2VQYWlyLmphdmE=) | `75.86% <0.00%> (-24.14%)` | :arrow_down: |
   | [.../helix/core/minion/MinionInstancesCleanupTask.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9NaW5pb25JbnN0YW5jZXNDbGVhbnVwVGFzay5qYXZh) | `57.14% <0.00%> (-14.29%)` | :arrow_down: |
   | [...impl/dictionary/DoubleOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRG91YmxlT25IZWFwTXV0YWJsZURpY3Rpb25hcnkuamF2YQ==) | `46.98% <0.00%> (-13.26%)` | :arrow_down: |
   | [...e/impl/dictionary/LongOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvTG9uZ09uSGVhcE11dGFibGVEaWN0aW9uYXJ5LmphdmE=) | `63.85% <0.00%> (-12.05%)` | :arrow_down: |
   | [.../impl/dictionary/LongOffHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvTG9uZ09mZkhlYXBNdXRhYmxlRGljdGlvbmFyeS5qYXZh) | `67.02% <0.00%> (-5.32%)` | :arrow_down: |
   | [...core/query/executor/ServerQueryExecutorV1Impl.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9leGVjdXRvci9TZXJ2ZXJRdWVyeUV4ZWN1dG9yVjFJbXBsLmphdmE=) | `79.45% <0.00%> (-4.87%)` | :arrow_down: |
   | [...cal/startree/v2/builder/BaseSingleTreeBuilder.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS92Mi9idWlsZGVyL0Jhc2VTaW5nbGVUcmVlQnVpbGRlci5qYXZh) | `88.39% <0.00%> (-4.47%)` | :arrow_down: |
   | [...pinot/core/query/request/context/TimerContext.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZXF1ZXN0L2NvbnRleHQvVGltZXJDb250ZXh0LmphdmE=) | `91.66% <0.00%> (-4.17%)` | :arrow_down: |
   | [...ot/segment/local/startree/OffHeapStarTreeNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS9PZmZIZWFwU3RhclRyZWVOb2RlLmphdmE=) | `81.48% <0.00%> (-3.71%)` | :arrow_down: |
   | ... and [17 more](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [06e7ac6...d35c45e](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] Jackie-Jiang commented on a change in pull request #7019: Resolve bug in HelixServerStarter for populating environment property…

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7019:
URL: https://github.com/apache/incubator-pinot/pull/7019#discussion_r645195262



##########
File path: pinot-plugins/pinot-environment/pinot-azure/src/main/java/org/apache/pinot/plugin/provider/AzureEnvironmentProvider.java
##########
@@ -40,21 +40,17 @@
 import org.apache.http.util.EntityUtils;
 import org.apache.pinot.spi.env.PinotConfiguration;
 import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 
 /**
  * Azure Environment Provider used to retrieve azure cloud specific instance configuration.
  */
 public class AzureEnvironmentProvider implements PinotEnvironmentProvider {
 
-  private static final Logger LOGGER = LoggerFactory.getLogger(AzureEnvironmentProvider.class);
-
   protected static final String MAX_RETRY = "maxRetry";
   protected static final String IMDS_ENDPOINT = "imdsEndpoint";
-  protected static final String CONNECTION_TIMEOUT = "connectionTimeout";
-  protected static final String REQUEST_TIMEOUT = "requestTimeout";
+  protected static final String CONNECTION_TIMEOUT_MILLIS = "connectionTimeoutMillis";
+  protected static final String REQUEST_TIMEOUT_MILLIS = "requestTimeoutMillis";

Review comment:
       To be consistent with other config keys in the code base
   ```suggestion
     protected static final String CONNECTION_TIMEOUT_MS = "connectionTimeoutMs";
     protected static final String REQUEST_TIMEOUT_MS = "requestTimeoutMs";
   ```




-- 
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] desaijay230592 commented on a change in pull request #7019: Resolve bug in HelixServerStarter for populating environment property…

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



##########
File path: pinot-plugins/pinot-environment/pinot-azure/src/main/java/org/apache/pinot/plugin/provider/AzureEnvironmentProvider.java
##########
@@ -40,21 +40,17 @@
 import org.apache.http.util.EntityUtils;
 import org.apache.pinot.spi.env.PinotConfiguration;
 import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 
 /**
  * Azure Environment Provider used to retrieve azure cloud specific instance configuration.
  */
 public class AzureEnvironmentProvider implements PinotEnvironmentProvider {
 
-  private static final Logger LOGGER = LoggerFactory.getLogger(AzureEnvironmentProvider.class);
-
   protected static final String MAX_RETRY = "maxRetry";
   protected static final String IMDS_ENDPOINT = "imdsEndpoint";
-  protected static final String CONNECTION_TIMEOUT = "connectionTimeout";
-  protected static final String REQUEST_TIMEOUT = "requestTimeout";
+  protected static final String CONNECTION_TIMEOUT_MILLIS = "connectionTimeoutMillis";
+  protected static final String REQUEST_TIMEOUT_MILLIS = "requestTimeoutMillis";

Review comment:
       @Jackie-Jiang  Saw some references where they are using MILLIS as suffix too like in StreamConfig.java.
   Let me know if its fine to keep MILLIS as suffix.Also I think using specifics will help. "MS" can be determined by some as micro seconds(less likely but possible), so using explicit unit mention would make it more clear. Thoughts ?   




-- 
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-commenter commented on pull request #7019: Resolve bug in HelixServerStarter for populating environment property…

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


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#7019](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d35c45e) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/06e7ac6e9e472dc9f96eec6733c02cbc3e6b0b15?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (06e7ac6) will **decrease** coverage by `7.88%`.
   > The diff coverage is `0.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/7019/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #7019      +/-   ##
   ============================================
   - Coverage     73.27%   65.39%   -7.89%     
     Complexity       12       12              
   ============================================
     Files          1452     1452              
     Lines         71960    71960              
     Branches      10417    10417              
   ============================================
   - Hits          52731    47060    -5671     
   - Misses        15702    21504    +5802     
   + Partials       3527     3396     -131     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration | `?` | |
   | unittests | `65.39% <0.00%> (-0.03%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...pinot/server/starter/helix/HelixServerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9IZWxpeFNlcnZlclN0YXJ0ZXIuamF2YQ==) | `0.00% <0.00%> (-49.04%)` | :arrow_down: |
   | [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...t/core/startree/plan/StarTreeDocIdSetPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlRG9jSWRTZXRQbGFuTm9kZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../core/startree/plan/StarTreeTransformPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlVHJhbnNmb3JtUGxhbk5vZGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...core/startree/plan/StarTreeProjectionPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlUHJvamVjdGlvblBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...t/minion/executor/MinionTaskZkMetadataManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhlY3V0b3IvTWluaW9uVGFza1prTWV0YWRhdGFNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [338 more](https://codecov.io/gh/apache/incubator-pinot/pull/7019/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [06e7ac6...d35c45e](https://codecov.io/gh/apache/incubator-pinot/pull/7019?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] desaijay230592 commented on a change in pull request #7019: Resolve bug in HelixServerStarter for populating environment property…

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



##########
File path: pinot-plugins/pinot-environment/pinot-azure/src/main/java/org/apache/pinot/plugin/provider/AzureEnvironmentProvider.java
##########
@@ -40,21 +40,17 @@
 import org.apache.http.util.EntityUtils;
 import org.apache.pinot.spi.env.PinotConfiguration;
 import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 
 /**
  * Azure Environment Provider used to retrieve azure cloud specific instance configuration.
  */
 public class AzureEnvironmentProvider implements PinotEnvironmentProvider {
 
-  private static final Logger LOGGER = LoggerFactory.getLogger(AzureEnvironmentProvider.class);
-
   protected static final String MAX_RETRY = "maxRetry";
   protected static final String IMDS_ENDPOINT = "imdsEndpoint";
-  protected static final String CONNECTION_TIMEOUT = "connectionTimeout";
-  protected static final String REQUEST_TIMEOUT = "requestTimeout";
+  protected static final String CONNECTION_TIMEOUT_MILLIS = "connectionTimeoutMillis";
+  protected static final String REQUEST_TIMEOUT_MILLIS = "requestTimeoutMillis";

Review comment:
       @Jackie-Jiang  Saw some references where they are using MILLIS as suffix too like in StreamConfig.java.
   Let me know if its fine to keep MILLIS as suffix.

##########
File path: pinot-plugins/pinot-environment/pinot-azure/src/main/java/org/apache/pinot/plugin/provider/AzureEnvironmentProvider.java
##########
@@ -40,21 +40,17 @@
 import org.apache.http.util.EntityUtils;
 import org.apache.pinot.spi.env.PinotConfiguration;
 import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 
 /**
  * Azure Environment Provider used to retrieve azure cloud specific instance configuration.
  */
 public class AzureEnvironmentProvider implements PinotEnvironmentProvider {
 
-  private static final Logger LOGGER = LoggerFactory.getLogger(AzureEnvironmentProvider.class);
-
   protected static final String MAX_RETRY = "maxRetry";
   protected static final String IMDS_ENDPOINT = "imdsEndpoint";
-  protected static final String CONNECTION_TIMEOUT = "connectionTimeout";
-  protected static final String REQUEST_TIMEOUT = "requestTimeout";
+  protected static final String CONNECTION_TIMEOUT_MILLIS = "connectionTimeoutMillis";
+  protected static final String REQUEST_TIMEOUT_MILLIS = "requestTimeoutMillis";

Review comment:
       @Jackie-Jiang  Saw some references where they are using MILLIS as suffix too like in StreamConfig.java.
   Let me know if its fine to keep MILLIS as suffix.Also I think using specifics will help. "MS" can be determined by some as micro seconds(less likely but possible), so using explicit unit mention would make it more clear. Thoughts ?   




-- 
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