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/04/26 18:15:31 UTC
[GitHub] [incubator-pinot] desaijay230592 opened a new pull request #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
desaijay230592 opened a new pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842
… Domain Information for the Server Instance
## Description
Creating generic interface PinotEnvironment which can be implemented by various cloud providers to customize the PinotConfiguration by setting different environment variables and other instance specific configuration.
Primary aim is to update the server instance config with failure/fault domain information which will be useful when designing segment assignment strategy to achieve high availability.
This change introduces a PinotEnvironment interface under pinot-spi(a.ka pinot-service-provider-interface). We introduce a pinot-enviroment/pinot-azure plugin containing AzureEnvironmentProvider class to provide azure specific pinot configuration customization which contains failure domain information. This failure domain information is retrieved from Azure IMDS (Instance Metadata Service) wherein call has to be made from within the VM.
Unit Tests are written for the same. End-End integration tests are in-progress. Will take LinkedIN Pinot-SRE help to conduct those.
Design Document: https://docs.google.com/document/d/1echXSSZfgKWLtkmA1KKxmdCRUkTqffqoJhNA-ovTfzc/
## 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
- New plugins added or old plugins removed
* [ ] 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
<!-- If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
-->
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
desaijay230592 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r623431698
##########
File path: pinot-plugins/pinot-environment/pinot-azure/pom.xml
##########
@@ -0,0 +1,65 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ 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.
+
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <artifactId>pinot-environment</artifactId>
+ <groupId>org.apache.pinot</groupId>
+ <version>0.7.0-SNAPSHOT</version>
Review comment:
I checked other pom files. They use the SNAPSHOT version itself. I realized I was using 0.7.0 instead of 0.8.0.
##########
File path: pinot-plugins/pinot-environment/pinot-azure/pom.xml
##########
@@ -0,0 +1,65 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ 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.
+
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <artifactId>pinot-environment</artifactId>
+ <groupId>org.apache.pinot</groupId>
+ <version>0.7.0-SNAPSHOT</version>
Review comment:
I checked other pom files. They use the SNAPSHOT version as well. I realized I was using 0.7.0 instead of 0.8.0.
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#issuecomment-834835328
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6842?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 [#6842](https://codecov.io/gh/apache/incubator-pinot/pull/6842?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a898a19) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/9b44c2164ad5ce87a6eb433ad90a7bbcb714fb0f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9b44c21) will **increase** coverage by `0.01%`.
> The diff coverage is `50.72%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6842?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 #6842 +/- ##
============================================
+ Coverage 73.52% 73.54% +0.01%
Complexity 12 12
============================================
Files 1428 1430 +2
Lines 70365 70423 +58
Branches 10154 10162 +8
============================================
+ Hits 51739 51795 +56
+ Misses 15211 15208 -3
- Partials 3415 3420 +5
```
| Flag | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| integration | `43.36% <10.14%> (+0.11%)` | `7.00 <0.00> (ø)` | |
| unittests | `65.45% <42.02%> (-0.03%)` | `12.00 <0.00> (ø)` | |
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/6842?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [.../environmentprovider/PinotEnvironmentProvider.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZW52aXJvbm1lbnRwcm92aWRlci9QaW5vdEVudmlyb25tZW50UHJvdmlkZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `31.48% <ø> (ø)` | `0.00 <0.00> (ø)` | |
| [...pinot/server/starter/helix/HelixServerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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==) | `50.71% <25.00%> (-1.57%)` | `0.00 <0.00> (ø)` | |
| [...quota/HelixExternalViewBasedQueryQuotaManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IZWxpeEV4dGVybmFsVmlld0Jhc2VkUXVlcnlRdW90YU1hbmFnZXIuamF2YQ==) | `68.81% <57.89%> (-2.18%)` | `0.00 <0.00> (ø)` | |
| [...nmentprovider/PinotEnvironmentProviderFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZW52aXJvbm1lbnRwcm92aWRlci9QaW5vdEVudmlyb25tZW50UHJvdmlkZXJGYWN0b3J5LmphdmE=) | `64.28% <64.28%> (ø)` | `0.00 <0.00> (?)` | |
| [.../BrokerResourceOnlineOfflineStateModelFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclJlc291cmNlT25saW5lT2ZmbGluZVN0YXRlTW9kZWxGYWN0b3J5LmphdmE=) | `55.81% <100.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...impl/dictionary/DoubleOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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%> (-7.23%)` | `0.00% <0.00%> (ø%)` | |
| [...mpl/dictionary/DoubleOffHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRG91YmxlT2ZmSGVhcE11dGFibGVEaWN0aW9uYXJ5LmphdmE=) | `57.44% <0.00%> (-6.39%)` | `0.00% <0.00%> (ø%)` | |
| [.../impl/dictionary/LongOffHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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%)` | `0.00% <0.00%> (ø%)` | |
| [...operator/filter/RangeIndexBasedFilterOperator.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvUmFuZ2VJbmRleEJhc2VkRmlsdGVyT3BlcmF0b3IuamF2YQ==) | `40.42% <0.00%> (-4.26%)` | `0.00% <0.00%> (ø%)` | |
| ... and [26 more](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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/6842?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/6842?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 [dce7479...a898a19](https://codecov.io/gh/apache/incubator-pinot/pull/6842?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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r631326435
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProvider.java
##########
@@ -0,0 +1,42 @@
+/**
+ * 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.environmentprovider;
+
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+/**
+ * Environment Provider interface implemented by different cloud providers to customize
+ * the base pinot configuration to add environment variables & instance specific configuration
+ */
+public interface PinotEnvironmentProvider {
+
+ String INSTANCE_FAILURE_DOMAIN = "pinot.environment.instance.failureDomain";
+
+ /**
+ * Customize base pinot configuration to add environment variables & instance specific configuration
+ * @return custom pinot configuration map
+ */
+ Map<String, Object> getEnvironment();
Review comment:
Let us say, we have two cloud implementations, returning their map that we write to instance config today:
Azure returns: {"failureDomain": "foo", "Mem": "1024GB"}
And cloudX returns {"failureDomain" : "bar", "Memory" : 64GB"}
Assume that at some point later we want to add functionality where pinot behaves depending on memory in the system (say, it routes more queries to servers having more memory, or allocatets more segments, or whatever).
What would pinot code look like? (and each provider will claim that they are already providing that information)
My guess is it would be like:
`
mem = map.get("MEM"); if (mem == null) {mem = map.get("MEMORY");}
`
Bad
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r631299443
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProvider.java
##########
@@ -0,0 +1,40 @@
+/**
+ * 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.environmentprovider;
+
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+/**
+ * Environment Provider interface implemented by different cloud providers to customize
+ * the base pinot configuration to add environment variables & instance specific configuration
+ */
+public interface PinotEnvironmentProvider {
+
+ /**
+ * Initializes the configurations specific to an environment provider.
+ */
+ void init(PinotConfiguration pinotConfiguration);
+
+ /**
+ * Customize base pinot configuration to add environment variables & instance specific configuration
+ * @return custom pinot configuration map
+ */
+ Map<String, String> getEnvironment();
Review comment:
So, every cloud provider can provide any random map, and we add the map to zookeeper? But then the only thing that Pinot can understand and work with, is failure domain. Can you give me an example of what Pinot could do if a cloud provider adds other keys to the map?
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#issuecomment-834835328
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6842?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 [#6842](https://codecov.io/gh/apache/incubator-pinot/pull/6842?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a898a19) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/9b44c2164ad5ce87a6eb433ad90a7bbcb714fb0f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9b44c21) will **decrease** coverage by `8.07%`.
> The diff coverage is `42.02%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6842?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 #6842 +/- ##
============================================
- Coverage 73.52% 65.45% -8.08%
Complexity 12 12
============================================
Files 1428 1430 +2
Lines 70365 70423 +58
Branches 10154 10162 +8
============================================
- Hits 51739 46098 -5641
- Misses 15211 21044 +5833
+ Partials 3415 3281 -134
```
| Flag | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| integration | `?` | `?` | |
| unittests | `65.45% <42.02%> (-0.03%)` | `12.00 <0.00> (ø)` | |
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/6842?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...pinot/server/starter/helix/HelixServerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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%> (-52.28%)` | `0.00 <0.00> (ø)` | |
| [.../environmentprovider/PinotEnvironmentProvider.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZW52aXJvbm1lbnRwcm92aWRlci9QaW5vdEVudmlyb25tZW50UHJvdmlkZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `31.48% <ø> (ø)` | `0.00 <0.00> (ø)` | |
| [...quota/HelixExternalViewBasedQueryQuotaManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IZWxpeEV4dGVybmFsVmlld0Jhc2VkUXVlcnlRdW90YU1hbmFnZXIuamF2YQ==) | `63.36% <52.63%> (-7.62%)` | `0.00 <0.00> (ø)` | |
| [...nmentprovider/PinotEnvironmentProviderFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZW52aXJvbm1lbnRwcm92aWRlci9QaW5vdEVudmlyb25tZW50UHJvdmlkZXJGYWN0b3J5LmphdmE=) | `64.28% <64.28%> (ø)` | `0.00 <0.00> (?)` | |
| [.../BrokerResourceOnlineOfflineStateModelFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclJlc291cmNlT25saW5lT2ZmbGluZVN0YXRlTW9kZWxGYWN0b3J5LmphdmE=) | `41.86% <100.00%> (-13.96%)` | `0.00 <0.00> (ø)` | |
| [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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%)` | `0.00% <0.00%> (ø%)` | |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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%)` | `0.00% <0.00%> (ø%)` | |
| [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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%)` | `0.00% <0.00%> (ø%)` | |
| [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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%)` | `0.00% <0.00%> (ø%)` | |
| ... and [342 more](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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/6842?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/6842?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 [dce7479...a898a19](https://codecov.io/gh/apache/incubator-pinot/pull/6842?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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
desaijay230592 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r626956969
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -321,6 +321,11 @@
public static final String CONFIG_OF_ENABLE_THREAD_CPU_TIME_MEASUREMENT =
"pinot.server.instance.enableThreadCpuTimeMeasurement";
public static final boolean DEFAULT_ENABLE_THREAD_CPU_TIME_MEASUREMENT = false;
+
+ // Environment Provider Configs
+ public static final String PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY = "pinot.server.environmentProvider.factory";
+ public static final String CONFIG_OF_ENABLE_ENVIRONMENT_PROVIDER_FACTORY = "pinot.server.environmentProvider.enabled";
+ public static final String CONFIG_OF_ENVIRONMENT_PROVIDER_TYPE = "pinot.server.environmentProvider.type";
Review comment:
Removed unnecessary variables. Introducing environmentProvider.className variable to determine which EnvironmentProvider class to invoke. Please see the recent changes.
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -103,9 +107,13 @@
public class HelixServerStarter implements ServiceStartable {
private static final Logger LOGGER = LoggerFactory.getLogger(HelixServerStarter.class);
+ private static final String ENVIRONMENT_IDENTIFIER = "environment";
+ private static final String FAILURE_DOMAIN_IDENTIFIER = "failureDomain";
Review comment:
Done.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -321,6 +321,11 @@
public static final String CONFIG_OF_ENABLE_THREAD_CPU_TIME_MEASUREMENT =
"pinot.server.instance.enableThreadCpuTimeMeasurement";
public static final boolean DEFAULT_ENABLE_THREAD_CPU_TIME_MEASUREMENT = false;
+
+ // Environment Provider Configs
+ public static final String PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY = "pinot.server.environmentProvider.factory";
+ public static final String CONFIG_OF_ENABLE_ENVIRONMENT_PROVIDER_FACTORY = "pinot.server.environmentProvider.enabled";
Review comment:
Removed
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
desaijay230592 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r630456447
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProvider.java
##########
@@ -0,0 +1,42 @@
+/**
+ * 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.environmentprovider;
+
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+/**
+ * Environment Provider interface implemented by different cloud providers to customize
+ * the base pinot configuration to add environment variables & instance specific configuration
+ */
+public interface PinotEnvironmentProvider {
+
+ String INSTANCE_FAILURE_DOMAIN = "pinot.environment.instance.failureDomain";
Review comment:
It is not specific to a server instance. It can b extended to other components too. Will move to CommonConstants.
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
desaijay230592 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r630603796
##########
File path: pinot-plugins/pinot-environment/pinot-azure/src/main/java/org/apache/pinot/plugin/provider/AzureEnvironmentProvider.java
##########
@@ -0,0 +1,172 @@
+/**
+ * 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.plugin.provider;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
+import java.util.HashMap;
+import java.util.Map;
+import javax.net.ssl.SSLException;
+import org.apache.commons.lang.StringUtils;
+import org.apache.http.HttpEntityEnclosingRequest;
+import org.apache.http.HttpStatus;
+import org.apache.http.StatusLine;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.protocol.HttpClientContext;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
+import org.apache.pinot.spi.utils.CommonConstants;
+
+
+/**
+ * Azure Environment Provider used to retrieve azure cloud specific instance configuration.
+ */
+public class AzureEnvironmentProvider implements PinotEnvironmentProvider {
+ 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";
+ private static final String COMPUTE = "compute";
+ private static final String METADATA = "Metadata";
+ private static final String PLATFORM_FAULT_DOMAIN = "platformFaultDomain";
+ private int _maxRetry;
+ private String _imdsEndpoint;
+ private CloseableHttpClient _closeableHttpClient;
+ private PinotConfiguration _serverConfigs;
+
+ public AzureEnvironmentProvider() {
+ }
+
+ public void init(PinotConfiguration pinotConfiguration) {
+ _serverConfigs = pinotConfiguration;
+ Preconditions.checkArgument(0 < Integer.parseInt(_serverConfigs.getProperty(MAX_RETRY)),
+ "[AzureEnvironmentProvider]: " + MAX_RETRY + " cannot be less than or equal to 0");
+ Preconditions.checkArgument(!StringUtils.isBlank(_serverConfigs.getProperty(IMDS_ENDPOINT)),
+ "[AzureEnvironmentProvider]: " + IMDS_ENDPOINT + " should not be null or empty");
+
+ _maxRetry = Integer.parseInt(_serverConfigs.getProperty(MAX_RETRY));
+ _imdsEndpoint = _serverConfigs.getProperty(IMDS_ENDPOINT);
+ int connectionTimeout = Integer.parseInt(_serverConfigs.getProperty(CONNECTION_TIMEOUT));
+ int requestTimeout = Integer.parseInt(_serverConfigs.getProperty(REQUEST_TIMEOUT));
+
+ final RequestConfig requestConfig = RequestConfig.custom()
+ .setConnectTimeout(connectionTimeout)
+ .setConnectionRequestTimeout(requestTimeout)
+ .build();
+
+ final HttpRequestRetryHandler httpRequestRetryHandler = (iOException, executionCount, httpContext) ->
+ !(executionCount >= _maxRetry
+ || iOException instanceof InterruptedIOException
+ || iOException instanceof UnknownHostException
+ || iOException instanceof SSLException
+ || HttpClientContext.adapt(httpContext).getRequest() instanceof HttpEntityEnclosingRequest);
+
+ _closeableHttpClient =
+ HttpClients.custom().setDefaultRequestConfig(requestConfig).setRetryHandler(httpRequestRetryHandler).build();
+ }
+
+ // Constructor for test purposes.
+ @VisibleForTesting
+ public AzureEnvironmentProvider(int maxRetry, String imdsEndpoint, CloseableHttpClient closeableHttpClient) {
+ _maxRetry = maxRetry;
+ _imdsEndpoint = imdsEndpoint;
+ _closeableHttpClient = Preconditions.checkNotNull(closeableHttpClient,
+ "[AzureEnvironmentProvider]: Closeable Http Client cannot be null");
+ }
+
+ /**
+ *
+ * Method for constructing custom pinot configuration used by the HelixServerStarter to update
+ * zookeeper node with custom instance configs.
+ * @return custom pinot configuration map
+ */
+ @Override
+ public Map<String, Object> getEnvironment() {
+ Map<String, Object> customPinotConfiguration = new HashMap<>(_serverConfigs.toMap());
Review comment:
Already made that change. Please refer to the latest diff :)
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
desaijay230592 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r630384926
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProvider.java
##########
@@ -0,0 +1,42 @@
+/**
+ * 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.environmentprovider;
+
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+/**
+ * Environment Provider interface implemented by different cloud providers to customize
+ * the base pinot configuration to add environment variables & instance specific configuration
+ */
+public interface PinotEnvironmentProvider {
+
+ String INSTANCE_FAILURE_DOMAIN = "pinot.environment.instance.failureDomain";
+
+ /**
+ * Customize base pinot configuration to add environment variables & instance specific configuration
+ * @return custom pinot configuration map
+ */
+ Map<String, Object> getEnvironment();
Review comment:
We are relying on parsing the map in the HelixServerStarter. Also the reason for returning a map is if we decide to add more cloud environment specific variables (for now its only failure domain) then we can leverage the map at Server Starter layer to make ZK instance config write/updates. We are maintaining a Map<String, Map<String, String>> which gets updated with the above environment properties and Pinot recognizes it to update the mapFields. So returning a Map from EnvironmentProvider should not be an issue.
--
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] xiangfu0 commented on a change in pull request #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
xiangfu0 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r622464520
##########
File path: pinot-plugins/pinot-environment/pinot-azure/pom.xml
##########
@@ -0,0 +1,65 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ 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.
+
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <artifactId>pinot-environment</artifactId>
+ <groupId>org.apache.pinot</groupId>
+ <version>0.7.0-SNAPSHOT</version>
Review comment:
you need to use version references here. see other pom files.
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
desaijay230592 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r630464367
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProviderFactory.java
##########
@@ -0,0 +1,82 @@
+/**
+ * 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.environmentprovider;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Iterables;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.plugin.PluginManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This factory class initializes the PinotEnvironmentProvider class.
+ * It creates a PinotEnvironment object based on the URI found.
+ */
+public class PinotEnvironmentProviderFactory {
+
+ private PinotEnvironmentProviderFactory() {
+ }
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(PinotEnvironmentProviderFactory.class);
+ private static final String CLASS = "class";
+ private static final Map<String, PinotEnvironmentProvider> PINOT_ENVIRONMENT_PROVIDER_MAP = new HashMap<>();
+
+ public static void init(PinotConfiguration environmentProviderFactoryConfig) {
+ // Get environment and it's respective class
+ PinotConfiguration environmentConfiguration = environmentProviderFactoryConfig.subset(CLASS);
+ List<String> environments = environmentConfiguration.getKeys();
+
+ if (environments.isEmpty()) {
+ LOGGER.info("Did not find any environment provider classes in the configuration");
+ return;
+ }
+
+ String environment = Iterables.getOnlyElement(environments);
+ String environmentProviderClassName = environmentConfiguration.getProperty(environment);
+ PinotConfiguration environmentProviderConfiguration = environmentProviderFactoryConfig.subset(environment);
+ LOGGER.info("Got environment {}, initializing class {}", environment, environmentProviderClassName);
+ register(environment, environmentProviderClassName, environmentProviderConfiguration);
+ }
+
+ // Utility to invoke the cloud specific environment provider.
+ public static PinotEnvironmentProvider getEnvironmentProvider(String environment) {
+ PinotEnvironmentProvider pinotEnvironmentProvider = PINOT_ENVIRONMENT_PROVIDER_MAP.get(environment);
+ Preconditions.checkState(pinotEnvironmentProvider != null,
+ "PinotEnvironmentProvider for environment: %s has not been initialized", environment);
+ return pinotEnvironmentProvider;
+ }
+
+ private static void register(
+ String environment, String environmentProviderFileName, PinotConfiguration environmentProviderConfiguration) {
+ try {
+ LOGGER.info("Initializing PinotEnvironmentProvider for environment {}, classname {}", environment, environmentProviderFileName);
+ PinotEnvironmentProvider pinotEnvironmentProvider = PluginManager.get().createInstance(environmentProviderFileName);
+ pinotEnvironmentProvider.init(environmentProviderConfiguration);
+ PINOT_ENVIRONMENT_PROVIDER_MAP.put(environment, pinotEnvironmentProvider);
Review comment:
Added a check if the map already contains the environment key. We will populate the map only for the first registration.
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
desaijay230592 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r631367717
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProvider.java
##########
@@ -0,0 +1,42 @@
+/**
+ * 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.environmentprovider;
+
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+/**
+ * Environment Provider interface implemented by different cloud providers to customize
+ * the base pinot configuration to add environment variables & instance specific configuration
+ */
+public interface PinotEnvironmentProvider {
+
+ String INSTANCE_FAILURE_DOMAIN = "pinot.environment.instance.failureDomain";
+
+ /**
+ * Customize base pinot configuration to add environment variables & instance specific configuration
+ * @return custom pinot configuration map
+ */
+ Map<String, Object> getEnvironment();
Review comment:
Summary:
- We want to move away from each environment provider returning a custom map of environment properties which then gets populated to the instance configs
- Will have separate method for each environment property that we want pinot to makes sense out of to update the instance configs.
- This approach of having separate interface will close the possible backdoor of populating the instance configs with garbage/unknown properties and only restrict to properties that pinot can make sense of.
- We will return default values for these properties in case an environment/cloud provider does not have the logic to fetch these properties in their core environment provider implementation.
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r631441793
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -218,6 +251,8 @@ private void registerServiceStatusHandler() {
}
private void updateInstanceConfigIfNeeded(String host, int port) {
+ Map<String, String> environmentProperties = new HashMap<>();
Review comment:
Move this to line 284
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -244,6 +279,24 @@ private void updateInstanceConfigIfNeeded(String host, int port) {
needToUpdateInstanceConfig = true;
}
+ // Update instance config with environment properties
+ if (_pinotEnvironmentProvider != null) {
+ // Retrieve failure domain information and add to the environment properties map
+ String failureDomain = _pinotEnvironmentProvider.getFailureDomain();
+ environmentProperties.put(CommonConstants.INSTANCE_FAILURE_DOMAIN, failureDomain);
+
+ // Fetch existing environment properties map from instance configs
+ Map<String, String> existingEnvironmentConfigsMap = instanceConfig.getRecord().getMapField(
+ CommonConstants.ENVIRONMENT_IDENTIFIER);
+
+ if (existingEnvironmentConfigsMap != null && !existingEnvironmentConfigsMap.equals(environmentProperties)) {
+ instanceConfig.getRecord().setMapField(CommonConstants.ENVIRONMENT_IDENTIFIER, environmentProperties);
+ LOGGER.info("Updated instance config for instance: {} with environment specific properties: {}",
Review comment:
The log message here (says "Updated...") and the one below (says "Updating....") are misleading in sequence. Suggest this change
```suggestion
LOGGER.info("Adding env properties for instance {} with environment specific properties: {}",
```
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java
##########
@@ -23,6 +23,10 @@
public class CommonConstants {
+ public static final String ENVIRONMENT_IDENTIFIER = "environment";
+ public static final String INSTANCE_FAILURE_DOMAIN = "failureDomain";
+ public static final String DEFAULT_FAILURE_DOMAIN = "-1";
Review comment:
```suggestion
public static final String DEFAULT_FAILURE_DOMAIN = "No such domain";
```
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProviderFactory.java
##########
@@ -0,0 +1,84 @@
+/**
+ * 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.environmentprovider;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Iterables;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.plugin.PluginManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This factory class initializes the PinotEnvironmentProvider class.
+ * It creates a PinotEnvironment object based on the URI found.
+ */
+public class PinotEnvironmentProviderFactory {
+
+ private PinotEnvironmentProviderFactory() {
+ }
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(PinotEnvironmentProviderFactory.class);
+ private static final String CLASS = "class";
+ private static final Map<String, PinotEnvironmentProvider> ENVIRONMENT_TO_ENVIRONMENT_PROVIDER_MAP = new HashMap<>();
Review comment:
Why do we map here? Do we expect one instance to be a member of multiple env providers?
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
desaijay230592 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r628396786
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -261,12 +270,39 @@ private void updateInstanceConfigIfNeeded(String host, int port) {
"Failed to update instance config");
}
+ // Fetch the overridden server configs for the invoked environment provider
+ private void populateFailureDomain() {
+ String className = _serverConf.getProperty(ENVIRONMENT_PROVIDER_CLASS_NAME);
+ if (className == null) return;
+ PinotEnvironmentProvider pinotEnvironmentProvider = PinotEnvironmentProviderFactory.getEnvironmentProvider(className.toLowerCase());
+ _serverConf = pinotEnvironmentProvider.getEnvironment(_serverConf.toMap());
+ Map<String, Object> overriddenPinotConfigurationMap = _serverConf.toMap();
+ String failureDomain = overriddenPinotConfigurationMap.containsKey(INSTANCE_FAILURE_DOMAIN.toLowerCase()) ?
+ String.valueOf(overriddenPinotConfigurationMap.get(INSTANCE_FAILURE_DOMAIN.toLowerCase())) : null;
+ if (failureDomain == null) {
+ LOGGER.info("No failure domain information found for instance: {}", _instanceId);
+ return;
+ }
+ Map<String, String> failureDomainMap = new HashMap<>();
+ failureDomainMap.put(FAILURE_DOMAIN_IDENTIFIER, failureDomain);
Review comment:
If u see we provide entire serverConfig map to getEnvironment() function wherein we add the fault domain property and return the entire config map. Putting all environment fields in the map is equivalent to storing all server config properties under mapFields which is not the intention here. Hence, we cherry-pick the Failure Domain information and only populate that in the mapFields. Yes, if going forward we want to populate more use-case specific environment properties we need to make some customizations.
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r630628874
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -261,6 +306,22 @@ private void updateInstanceConfigIfNeeded(String host, int port) {
"Failed to update instance config");
}
+ // Update instance configs with environment specific properties
+ private void updateInstanceConfigWithEnvironmentSpecificConfigs() {
Review comment:
I was not aware of this change. It is not reflected in the design doc either. Can you make it consistent please?
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r629786020
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProviderFactory.java
##########
@@ -0,0 +1,82 @@
+/**
+ * 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.environmentprovider;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Iterables;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.plugin.PluginManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This factory class initializes the PinotEnvironmentProvider class.
+ * It creates a PinotEnvironment object based on the URI found.
+ */
+public class PinotEnvironmentProviderFactory {
+
+ private PinotEnvironmentProviderFactory() {
+ }
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(PinotEnvironmentProviderFactory.class);
+ private static final String CLASS = "class";
+ private static final Map<String, PinotEnvironmentProvider> PINOT_ENVIRONMENT_PROVIDER_MAP = new HashMap<>();
+
+ public static void init(PinotConfiguration environmentProviderFactoryConfig) {
+ // Get environment and it's respective class
+ PinotConfiguration environmentConfiguration = environmentProviderFactoryConfig.subset(CLASS);
+ List<String> environments = environmentConfiguration.getKeys();
+
+ if (environments.isEmpty()) {
+ LOGGER.info("Did not find any environment provider classes in the configuration");
+ return;
+ }
+
+ String environment = Iterables.getOnlyElement(environments);
+ String environmentProviderClassName = environmentConfiguration.getProperty(environment);
+ PinotConfiguration environmentProviderConfiguration = environmentProviderFactoryConfig.subset(environment);
+ LOGGER.info("Got environment {}, initializing class {}", environment, environmentProviderClassName);
+ register(environment, environmentProviderClassName, environmentProviderConfiguration);
+ }
+
+ // Utility to invoke the cloud specific environment provider.
+ public static PinotEnvironmentProvider getEnvironmentProvider(String environment) {
+ PinotEnvironmentProvider pinotEnvironmentProvider = PINOT_ENVIRONMENT_PROVIDER_MAP.get(environment);
+ Preconditions.checkState(pinotEnvironmentProvider != null,
+ "PinotEnvironmentProvider for environment: %s has not been initialized", environment);
+ return pinotEnvironmentProvider;
+ }
+
+ private static void register(
+ String environment, String environmentProviderFileName, PinotConfiguration environmentProviderConfiguration) {
+ try {
+ LOGGER.info("Initializing PinotEnvironmentProvider for environment {}, classname {}", environment, environmentProviderFileName);
+ PinotEnvironmentProvider pinotEnvironmentProvider = PluginManager.get().createInstance(environmentProviderFileName);
+ pinotEnvironmentProvider.init(environmentProviderConfiguration);
+ PINOT_ENVIRONMENT_PROVIDER_MAP.put(environment, pinotEnvironmentProvider);
Review comment:
It may be a good idea to at least detect duplicate registrations and register either the first or last one (could be useful in an Integration test env where we may start more than one server, for example)
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProviderFactory.java
##########
@@ -0,0 +1,82 @@
+/**
+ * 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.environmentprovider;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Iterables;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.plugin.PluginManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This factory class initializes the PinotEnvironmentProvider class.
+ * It creates a PinotEnvironment object based on the URI found.
+ */
+public class PinotEnvironmentProviderFactory {
+
+ private PinotEnvironmentProviderFactory() {
+ }
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(PinotEnvironmentProviderFactory.class);
+ private static final String CLASS = "class";
+ private static final Map<String, PinotEnvironmentProvider> PINOT_ENVIRONMENT_PROVIDER_MAP = new HashMap<>();
Review comment:
```suggestion
private static final Map<String, PinotEnvironmentProvider> ENVNAME_2_ENVPROVIDER_MAP = new HashMap<>();
```
I am assuming the key is env name?
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProvider.java
##########
@@ -0,0 +1,42 @@
+/**
+ * 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.environmentprovider;
+
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+/**
+ * Environment Provider interface implemented by different cloud providers to customize
+ * the base pinot configuration to add environment variables & instance specific configuration
+ */
+public interface PinotEnvironmentProvider {
+
+ String INSTANCE_FAILURE_DOMAIN = "pinot.environment.instance.failureDomain";
+
+ /**
+ * Customize base pinot configuration to add environment variables & instance specific configuration
+ * @return custom pinot configuration map
+ */
+ Map<String, Object> getEnvironment();
Review comment:
Why return a map from the Env provider? Map is how Pinot chooses to store objects into zookeeper instance config. If we return a map from here, Pinot does not know how to parse the map.
Instead of this, I would declare a specific call like getFailureDomain(). Also, another call getFailureDomain(NodeName).
In the first case, it returns a string with the failure domain name of the node in which the call is made.
In the second case, it returns a string with the failure domain name of the specified node.
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProviderFactory.java
##########
@@ -0,0 +1,82 @@
+/**
+ * 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.environmentprovider;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Iterables;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.plugin.PluginManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This factory class initializes the PinotEnvironmentProvider class.
+ * It creates a PinotEnvironment object based on the URI found.
+ */
+public class PinotEnvironmentProviderFactory {
+
+ private PinotEnvironmentProviderFactory() {
+ }
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(PinotEnvironmentProviderFactory.class);
+ private static final String CLASS = "class";
+ private static final Map<String, PinotEnvironmentProvider> PINOT_ENVIRONMENT_PROVIDER_MAP = new HashMap<>();
+
+ public static void init(PinotConfiguration environmentProviderFactoryConfig) {
+ // Get environment and it's respective class
+ PinotConfiguration environmentConfiguration = environmentProviderFactoryConfig.subset(CLASS);
+ List<String> environments = environmentConfiguration.getKeys();
+
+ if (environments.isEmpty()) {
+ LOGGER.info("Did not find any environment provider classes in the configuration");
+ return;
+ }
+
+ String environment = Iterables.getOnlyElement(environments);
+ String environmentProviderClassName = environmentConfiguration.getProperty(environment);
+ PinotConfiguration environmentProviderConfiguration = environmentProviderFactoryConfig.subset(environment);
+ LOGGER.info("Got environment {}, initializing class {}", environment, environmentProviderClassName);
+ register(environment, environmentProviderClassName, environmentProviderConfiguration);
+ }
+
+ // Utility to invoke the cloud specific environment provider.
+ public static PinotEnvironmentProvider getEnvironmentProvider(String environment) {
+ PinotEnvironmentProvider pinotEnvironmentProvider = PINOT_ENVIRONMENT_PROVIDER_MAP.get(environment);
+ Preconditions.checkState(pinotEnvironmentProvider != null,
+ "PinotEnvironmentProvider for environment: %s has not been initialized", environment);
+ return pinotEnvironmentProvider;
+ }
+
+ private static void register(
+ String environment, String environmentProviderFileName, PinotConfiguration environmentProviderConfiguration) {
Review comment:
```suggestion
String environment, String environmentProviderClassName, PinotConfiguration environmentProviderConfiguration) {
```
##########
File path: pinot-plugins/pinot-environment/pinot-azure/src/main/java/org/apache/pinot/plugin/provider/AzureEnvironmentProvider.java
##########
@@ -0,0 +1,171 @@
+/**
+ * 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.plugin.provider;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
+import java.util.HashMap;
+import java.util.Map;
+import javax.net.ssl.SSLException;
+import javax.ws.rs.WebApplicationException;
+import org.apache.commons.lang.StringUtils;
+import org.apache.http.HttpEntityEnclosingRequest;
+import org.apache.http.HttpStatus;
+import org.apache.http.StatusLine;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.protocol.HttpClientContext;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
+
+/**
+ * Azure Environment Provider used to retrieve azure cloud specific instance configuration.
+ */
+public class AzureEnvironmentProvider implements PinotEnvironmentProvider {
+ 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";
+ private static final String COMPUTE = "compute";
+ private static final String METADATA = "Metadata";
+ private static final String PLATFORM_FAULT_DOMAIN = "platformFaultDomain";
+ private int _maxRetry;
+ private String _imdsEndpoint;
+ private CloseableHttpClient _closeableHttpClient;
+ private PinotConfiguration _serverConfigs;
+
+ public AzureEnvironmentProvider() {
+ }
+
+ public void init(PinotConfiguration pinotConfiguration) throws NullPointerException, IllegalArgumentException {
+ _serverConfigs = pinotConfiguration;
+ Preconditions.checkArgument(0 < Integer.parseInt(_serverConfigs.getProperty(MAX_RETRY)),
+ "maxRetry cannot be less than or equal to 0");
+ Preconditions.checkArgument(!StringUtils.isBlank(_serverConfigs.getProperty(IMDS_ENDPOINT)),
+ "imdsEndpoint should not be null or empty");
+
+ _maxRetry = Integer.parseInt(_serverConfigs.getProperty(MAX_RETRY));
+ _imdsEndpoint = _serverConfigs.getProperty(IMDS_ENDPOINT);
+ int connectionTimeout = Integer.parseInt(_serverConfigs.getProperty(CONNECTION_TIMEOUT));
+ int requestTimeout = Integer.parseInt(_serverConfigs.getProperty(REQUEST_TIMEOUT));
+
+ final RequestConfig requestConfig = RequestConfig.custom()
+ .setConnectTimeout(connectionTimeout)
+ .setConnectionRequestTimeout(requestTimeout)
+ .build();
+
+ final HttpRequestRetryHandler httpRequestRetryHandler = (iOException, executionCount, httpContext) ->
+ !(executionCount >= _maxRetry
+ || iOException instanceof InterruptedIOException
+ || iOException instanceof UnknownHostException
+ || iOException instanceof SSLException
+ || HttpClientContext.adapt(httpContext).getRequest() instanceof HttpEntityEnclosingRequest);
+
+ _closeableHttpClient =
+ HttpClients.custom().setDefaultRequestConfig(requestConfig).setRetryHandler(httpRequestRetryHandler).build();
+ }
+
+ // Constructor for test purposes.
+ public AzureEnvironmentProvider(int maxRetry, String imdsEndpoint, CloseableHttpClient closeableHttpClient) {
+ Preconditions.checkArgument(maxRetry > 0, "maxRetry cannot be less than or equal to 0");
+ Preconditions.checkArgument(!StringUtils.isBlank(imdsEndpoint), "imdsEndpoint should not be null or empty");
+ _maxRetry = maxRetry;
+ _imdsEndpoint = imdsEndpoint;
+ _closeableHttpClient = Preconditions.checkNotNull(closeableHttpClient, "Closeable Http Client cannot be null");
+ }
+
+ /**
+ *
+ * Method for constructing custom pinot configuration used by the HelixServerStarter to update
+ * zookeeper node with custom instance configs.
+ * @return custom pinot configuration map
+ */
+ @Override
+ public Map<String, Object> getEnvironment() {
+ Map<String, Object> customPinotConfiguration = new HashMap<>(_serverConfigs.toMap());
+
+ // Populate failure domain information
+ customPinotConfiguration.put(INSTANCE_FAILURE_DOMAIN, getFailureDomain());
+
+ return customPinotConfiguration;
+ }
+
+ // Utility used to query the azure instance metadata service (Azure IMDS) to fetch the fault/failure domain information.
+ @VisibleForTesting
+ protected final String getFailureDomain() {
+ final String responsePayload = getAzureInstanceMetadata();
+
+ // For a sample response payload,
+ // check https://docs.microsoft.com/en-us/azure/virtual-machines/windows/instance-metadata-service?tabs=linux
+ final ObjectMapper objectMapper = new ObjectMapper();
+ try {
+ final JsonNode jsonNode = objectMapper.readTree(responsePayload);
+ final JsonNode computeNode = jsonNode.path(COMPUTE);
+
+ if (computeNode.isMissingNode()) {
+ throw new WebApplicationException(
Review comment:
Why is this throwing WedApplicationException? Please change it to RuntimeException or a specific UnknownEnvException.
Also, you need to decide what HelixServerStarter should do if it gets an exception from this call. Do you want to proceed to start the service without failure domain, or block the server from starting?
##########
File path: pinot-plugins/pinot-environment/pinot-azure/src/main/java/org/apache/pinot/plugin/provider/AzureEnvironmentProvider.java
##########
@@ -0,0 +1,171 @@
+/**
+ * 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.plugin.provider;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
+import java.util.HashMap;
+import java.util.Map;
+import javax.net.ssl.SSLException;
+import javax.ws.rs.WebApplicationException;
+import org.apache.commons.lang.StringUtils;
+import org.apache.http.HttpEntityEnclosingRequest;
+import org.apache.http.HttpStatus;
+import org.apache.http.StatusLine;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.protocol.HttpClientContext;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
+
+/**
+ * Azure Environment Provider used to retrieve azure cloud specific instance configuration.
+ */
+public class AzureEnvironmentProvider implements PinotEnvironmentProvider {
+ 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";
+ private static final String COMPUTE = "compute";
+ private static final String METADATA = "Metadata";
+ private static final String PLATFORM_FAULT_DOMAIN = "platformFaultDomain";
+ private int _maxRetry;
+ private String _imdsEndpoint;
+ private CloseableHttpClient _closeableHttpClient;
+ private PinotConfiguration _serverConfigs;
+
+ public AzureEnvironmentProvider() {
+ }
+
+ public void init(PinotConfiguration pinotConfiguration) throws NullPointerException, IllegalArgumentException {
+ _serverConfigs = pinotConfiguration;
+ Preconditions.checkArgument(0 < Integer.parseInt(_serverConfigs.getProperty(MAX_RETRY)),
+ "maxRetry cannot be less than or equal to 0");
+ Preconditions.checkArgument(!StringUtils.isBlank(_serverConfigs.getProperty(IMDS_ENDPOINT)),
+ "imdsEndpoint should not be null or empty");
+
+ _maxRetry = Integer.parseInt(_serverConfigs.getProperty(MAX_RETRY));
+ _imdsEndpoint = _serverConfigs.getProperty(IMDS_ENDPOINT);
+ int connectionTimeout = Integer.parseInt(_serverConfigs.getProperty(CONNECTION_TIMEOUT));
+ int requestTimeout = Integer.parseInt(_serverConfigs.getProperty(REQUEST_TIMEOUT));
+
+ final RequestConfig requestConfig = RequestConfig.custom()
+ .setConnectTimeout(connectionTimeout)
+ .setConnectionRequestTimeout(requestTimeout)
+ .build();
+
+ final HttpRequestRetryHandler httpRequestRetryHandler = (iOException, executionCount, httpContext) ->
+ !(executionCount >= _maxRetry
+ || iOException instanceof InterruptedIOException
+ || iOException instanceof UnknownHostException
+ || iOException instanceof SSLException
+ || HttpClientContext.adapt(httpContext).getRequest() instanceof HttpEntityEnclosingRequest);
+
+ _closeableHttpClient =
+ HttpClients.custom().setDefaultRequestConfig(requestConfig).setRetryHandler(httpRequestRetryHandler).build();
+ }
+
+ // Constructor for test purposes.
+ public AzureEnvironmentProvider(int maxRetry, String imdsEndpoint, CloseableHttpClient closeableHttpClient) {
+ Preconditions.checkArgument(maxRetry > 0, "maxRetry cannot be less than or equal to 0");
+ Preconditions.checkArgument(!StringUtils.isBlank(imdsEndpoint), "imdsEndpoint should not be null or empty");
+ _maxRetry = maxRetry;
+ _imdsEndpoint = imdsEndpoint;
+ _closeableHttpClient = Preconditions.checkNotNull(closeableHttpClient, "Closeable Http Client cannot be null");
+ }
+
+ /**
+ *
+ * Method for constructing custom pinot configuration used by the HelixServerStarter to update
+ * zookeeper node with custom instance configs.
+ * @return custom pinot configuration map
+ */
+ @Override
+ public Map<String, Object> getEnvironment() {
Review comment:
See comments in the interface
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
desaijay230592 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r628488447
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -261,12 +270,39 @@ private void updateInstanceConfigIfNeeded(String host, int port) {
"Failed to update instance config");
}
+ // Fetch the overridden server configs for the invoked environment provider
+ private void populateFailureDomain() {
+ String className = _serverConf.getProperty(ENVIRONMENT_PROVIDER_CLASS_NAME);
+ if (className == null) return;
Review comment:
Done.
--
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] amarathavale commented on a change in pull request #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
amarathavale commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r630721982
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -154,6 +161,36 @@ public HelixServerStarter(String helixClusterName, String zkAddress, PinotConfig
Server.DEFAULT_CURRENT_DATA_TABLE_VERSION));
}
+ /**
+ * Invoke pinot environment provider factory's init method to register the environment provider &
+ * fetch the overridden server configs for the invoked environment provider.
+ */
+ @Nullable
+ private Map<String, String> fetchEnvironmentProperties() {
+ PinotConfiguration environmentProviderConfigs = _serverConf.subset(
+ Server.PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY);
+
+ if (environmentProviderConfigs.toMap().isEmpty()) {
+ LOGGER.info("No environment provider config values provided for server property: {}",
+ Server.PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY);
+ return null;
Review comment:
A null return type for a map leads to 3 states: null/empty/non-empty. I see you are handling the null case, but returning an empty map would imply the same thing? i.e. there are no overrides?
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
desaijay230592 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r630636007
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -261,6 +306,22 @@ private void updateInstanceConfigIfNeeded(String host, int port) {
"Failed to update instance config");
}
+ // Update instance configs with environment specific properties
+ private void updateInstanceConfigWithEnvironmentSpecificConfigs() {
Review comment:
Updating the document for consistency.
--
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] xiangfu0 commented on a change in pull request #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
xiangfu0 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r627738681
##########
File path: pinot-plugins/pinot-environment/pinot-azure/pom.xml
##########
@@ -0,0 +1,65 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ 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.
+
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <artifactId>pinot-environment</artifactId>
+ <groupId>org.apache.pinot</groupId>
+ <version>0.8.0-SNAPSHOT</version>
+ <relativePath>..</relativePath>
+ </parent>
+ <artifactId>pinot-azure</artifactId>
+ <name>Pinot Azure Environment</name>
+ <url>https://pinot.apache.org/</url>
+ <properties>
+ <pinot.root>${basedir}/../../..</pinot.root>
+ <phase.prop>package</phase.prop>
+ </properties>
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-spi</artifactId>
+ <version>0.8.0-SNAPSHOT</version>
Review comment:
`<version>${project.version}</version>`
##########
File path: pinot-plugins/pinot-environment/pinot-azure/pom.xml
##########
@@ -0,0 +1,65 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ 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.
+
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <artifactId>pinot-environment</artifactId>
+ <groupId>org.apache.pinot</groupId>
+ <version>0.8.0-SNAPSHOT</version>
+ <relativePath>..</relativePath>
+ </parent>
+ <artifactId>pinot-azure</artifactId>
+ <name>Pinot Azure Environment</name>
+ <url>https://pinot.apache.org/</url>
+ <properties>
+ <pinot.root>${basedir}/../../..</pinot.root>
+ <phase.prop>package</phase.prop>
+ </properties>
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-spi</artifactId>
+ <version>0.8.0-SNAPSHOT</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-common</artifactId>
+ <version>0.8.0-SNAPSHOT</version>
Review comment:
`<version>${project.version}</version>`
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#issuecomment-834835328
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6842?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 [#6842](https://codecov.io/gh/apache/incubator-pinot/pull/6842?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1ba47bd) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/bb2519494a15a3125dd509286d60ffd124f91c37?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (bb25194) will **decrease** coverage by `8.06%`.
> The diff coverage is `30.00%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6842?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 #6842 +/- ##
============================================
- Coverage 73.55% 65.49% -8.07%
Complexity 12 12
============================================
Files 1423 1424 +1
Lines 70029 70089 +60
Branches 10119 10125 +6
============================================
- Hits 51509 45902 -5607
- Misses 15103 20904 +5801
+ Partials 3417 3283 -134
```
| Flag | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| integration | `?` | `?` | |
| unittests | `65.49% <30.00%> (-0.01%)` | `12.00 <0.00> (ø)` | |
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/6842?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...pinot/server/starter/helix/HelixServerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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%> (-52.28%)` | `0.00 <0.00> (ø)` | |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `31.48% <ø> (ø)` | `0.00 <0.00> (ø)` | |
| [...nmentprovider/PinotEnvironmentProviderFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZW52aXJvbm1lbnRwcm92aWRlci9QaW5vdEVudmlyb25tZW50UHJvdmlkZXJGYWN0b3J5LmphdmE=) | `66.66% <66.66%> (ø)` | `0.00 <0.00> (?)` | |
| [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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%)` | `0.00% <0.00%> (ø%)` | |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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%)` | `0.00% <0.00%> (ø%)` | |
| [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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%)` | `0.00% <0.00%> (ø%)` | |
| [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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%)` | `0.00% <0.00%> (ø%)` | |
| [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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%)` | `0.00% <0.00%> (ø%)` | |
| [...t/core/startree/plan/StarTreeDocIdSetPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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%)` | `0.00% <0.00%> (ø%)` | |
| [.../core/startree/plan/StarTreeTransformPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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%)` | `0.00% <0.00%> (ø%)` | |
| ... and [342 more](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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/6842?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/6842?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 [bb25194...1ba47bd](https://codecov.io/gh/apache/incubator-pinot/pull/6842?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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r628638866
##########
File path: pinot-plugins/pinot-environment/pinot-azure/src/main/java/org/apache/pinot/plugin/provider/AzureEnvironmentProvider.java
##########
@@ -0,0 +1,171 @@
+/**
+ * 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.plugin.provider;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
+import java.util.HashMap;
+import java.util.Map;
+import javax.net.ssl.SSLException;
+import javax.ws.rs.WebApplicationException;
+import org.apache.commons.lang.StringUtils;
+import org.apache.http.HttpEntityEnclosingRequest;
+import org.apache.http.HttpStatus;
+import org.apache.http.StatusLine;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.protocol.HttpClientContext;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
+
+/**
+ * Azure Environment Provider used to retrieve azure cloud specific instance configuration.
+ */
+public class AzureEnvironmentProvider implements PinotEnvironmentProvider {
+ 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";
+ private static final String COMPUTE = "compute";
+ private static final String METADATA = "Metadata";
+ private static final String PLATFORM_FAULT_DOMAIN = "platformFaultDomain";
+ private int _maxRetry;
+ private String _imdsEndpoint;
+ private CloseableHttpClient _closeableHttpClient;
+ private PinotConfiguration _serverConfigs;
+
+ public AzureEnvironmentProvider() {
+ }
+
+ public void init(PinotConfiguration pinotConfiguration) throws NullPointerException, IllegalArgumentException {
+ _serverConfigs = pinotConfiguration;
+ Preconditions.checkArgument(0 < Integer.parseInt(_serverConfigs.getProperty(MAX_RETRY)),
Review comment:
Please include the keyname in your error/exception message. There are many configuration values, and we don't want a user asking in the slack channel which retry we are referring to.
I would suggest a standard error message like this: "AzureEnvironmentProvider: <key> should be <some expression>". And yes, please use the property name from the key.
##########
File path: pinot-plugins/pinot-environment/pinot-azure/src/main/java/org/apache/pinot/plugin/provider/AzureEnvironmentProvider.java
##########
@@ -0,0 +1,171 @@
+/**
+ * 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.plugin.provider;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
+import java.util.HashMap;
+import java.util.Map;
+import javax.net.ssl.SSLException;
+import javax.ws.rs.WebApplicationException;
+import org.apache.commons.lang.StringUtils;
+import org.apache.http.HttpEntityEnclosingRequest;
+import org.apache.http.HttpStatus;
+import org.apache.http.StatusLine;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.protocol.HttpClientContext;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
+
+/**
+ * Azure Environment Provider used to retrieve azure cloud specific instance configuration.
+ */
+public class AzureEnvironmentProvider implements PinotEnvironmentProvider {
+ 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";
+ private static final String COMPUTE = "compute";
+ private static final String METADATA = "Metadata";
+ private static final String PLATFORM_FAULT_DOMAIN = "platformFaultDomain";
+ private int _maxRetry;
+ private String _imdsEndpoint;
+ private CloseableHttpClient _closeableHttpClient;
+ private PinotConfiguration _serverConfigs;
+
+ public AzureEnvironmentProvider() {
+ }
+
+ public void init(PinotConfiguration pinotConfiguration) throws NullPointerException, IllegalArgumentException {
+ _serverConfigs = pinotConfiguration;
+ Preconditions.checkArgument(0 < Integer.parseInt(_serverConfigs.getProperty(MAX_RETRY)),
+ "maxRetry cannot be less than or equal to 0");
+ Preconditions.checkArgument(!StringUtils.isBlank(_serverConfigs.getProperty(IMDS_ENDPOINT)),
+ "imdsEndpoint should not be null or empty");
+
+ _maxRetry = Integer.parseInt(_serverConfigs.getProperty(MAX_RETRY));
+ _imdsEndpoint = _serverConfigs.getProperty(IMDS_ENDPOINT);
+ int connectionTimeout = Integer.parseInt(_serverConfigs.getProperty(CONNECTION_TIMEOUT));
+ int requestTimeout = Integer.parseInt(_serverConfigs.getProperty(REQUEST_TIMEOUT));
+
+ final RequestConfig requestConfig = RequestConfig.custom()
+ .setConnectTimeout(connectionTimeout)
+ .setConnectionRequestTimeout(requestTimeout)
+ .build();
+
+ final HttpRequestRetryHandler httpRequestRetryHandler = (iOException, executionCount, httpContext) ->
+ !(executionCount >= _maxRetry
+ || iOException instanceof InterruptedIOException
+ || iOException instanceof UnknownHostException
+ || iOException instanceof SSLException
+ || HttpClientContext.adapt(httpContext).getRequest() instanceof HttpEntityEnclosingRequest);
+
+ _closeableHttpClient =
+ HttpClients.custom().setDefaultRequestConfig(requestConfig).setRetryHandler(httpRequestRetryHandler).build();
+ }
+
+ // Constructor for test purposes.
Review comment:
Please annotate as @VisibleForTesting
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -154,6 +160,45 @@ public HelixServerStarter(String helixClusterName, String zkAddress, PinotConfig
Server.DEFAULT_CURRENT_DATA_TABLE_VERSION));
}
+ /**
+ * Invoke pinot environment provider factory's init method to register the environment provider &
+ * fetch the overridden server configs for the invoked environment provider.
+ */
+ private void fetchEnvironmentSpecificConfigs() {
+ PinotConfiguration environmentProviderConfigs = _serverConf.subset(
+ Server.PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY);
+
+ if (environmentProviderConfigs.toMap().isEmpty()) {
+ LOGGER.info("No Environment Provider Configs found.");
+ return;
+ }
+
+ PinotEnvironmentProviderFactory.init(environmentProviderConfigs);
+ String environmentProviderClassName = _serverConf.getProperty(Server.ENVIRONMENT_PROVIDER_CLASS_NAME);
+ if (environmentProviderClassName == null) {
+ LOGGER.info("No Class Name provided for Environment Provider.");
+ return;
+ }
+
+ // Fetch environment provider instance
+ PinotEnvironmentProvider pinotEnvironmentProvider = PinotEnvironmentProviderFactory.getEnvironmentProvider(
+ environmentProviderClassName.toLowerCase());
+
+ // Fetch the overridden pinot configuration containing custom configs.
+ Map<String, Object> overriddenPinotConfigs = pinotEnvironmentProvider.getEnvironment();
+
+ // Populate environment specific fields in the map.
+ String failureDomain = (String)overriddenPinotConfigs.get(PinotEnvironmentProvider.INSTANCE_FAILURE_DOMAIN);
+ if (failureDomain == null) {
+ LOGGER.info("No failure domain information found for instance: {}", _instanceId);
Review comment:
Please add the class name that we used to get the fault domain info. That way, an operator will know which class to fix/update.
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -154,6 +160,45 @@ public HelixServerStarter(String helixClusterName, String zkAddress, PinotConfig
Server.DEFAULT_CURRENT_DATA_TABLE_VERSION));
}
+ /**
+ * Invoke pinot environment provider factory's init method to register the environment provider &
+ * fetch the overridden server configs for the invoked environment provider.
+ */
+ private void fetchEnvironmentSpecificConfigs() {
+ PinotConfiguration environmentProviderConfigs = _serverConf.subset(
+ Server.PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY);
+
+ if (environmentProviderConfigs.toMap().isEmpty()) {
+ LOGGER.info("No Environment Provider Configs found.");
+ return;
+ }
+
+ PinotEnvironmentProviderFactory.init(environmentProviderConfigs);
+ String environmentProviderClassName = _serverConf.getProperty(Server.ENVIRONMENT_PROVIDER_CLASS_NAME);
+ if (environmentProviderClassName == null) {
+ LOGGER.info("No Class Name provided for Environment Provider.");
Review comment:
Add the property name in the log msg please
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProvider.java
##########
@@ -0,0 +1,42 @@
+/**
+ * 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.environmentprovider;
+
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+/**
+ * Environment Provider interface implemented by different cloud providers to customize
+ * the base pinot configuration to add environment variables & instance specific configuration
+ */
+public interface PinotEnvironmentProvider {
+
+ String INSTANCE_FAILURE_DOMAIN = "pinot.environment.instance.failureDomain";
Review comment:
This is a top level config? If so, should it go with the other configs? Or, is is "pinot.server.pinot.environment.instance..." (in which case the second pinot seems to be unnecessary)?
##########
File path: pinot-plugins/pinot-environment/pinot-azure/src/main/java/org/apache/pinot/plugin/provider/AzureEnvironmentProvider.java
##########
@@ -0,0 +1,171 @@
+/**
+ * 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.plugin.provider;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
+import java.util.HashMap;
+import java.util.Map;
+import javax.net.ssl.SSLException;
+import javax.ws.rs.WebApplicationException;
+import org.apache.commons.lang.StringUtils;
+import org.apache.http.HttpEntityEnclosingRequest;
+import org.apache.http.HttpStatus;
+import org.apache.http.StatusLine;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.protocol.HttpClientContext;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
+
+/**
+ * Azure Environment Provider used to retrieve azure cloud specific instance configuration.
+ */
+public class AzureEnvironmentProvider implements PinotEnvironmentProvider {
+ 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";
+ private static final String COMPUTE = "compute";
+ private static final String METADATA = "Metadata";
+ private static final String PLATFORM_FAULT_DOMAIN = "platformFaultDomain";
+ private int _maxRetry;
+ private String _imdsEndpoint;
+ private CloseableHttpClient _closeableHttpClient;
+ private PinotConfiguration _serverConfigs;
+
+ public AzureEnvironmentProvider() {
+ }
+
+ public void init(PinotConfiguration pinotConfiguration) throws NullPointerException, IllegalArgumentException {
+ _serverConfigs = pinotConfiguration;
+ Preconditions.checkArgument(0 < Integer.parseInt(_serverConfigs.getProperty(MAX_RETRY)),
+ "maxRetry cannot be less than or equal to 0");
+ Preconditions.checkArgument(!StringUtils.isBlank(_serverConfigs.getProperty(IMDS_ENDPOINT)),
+ "imdsEndpoint should not be null or empty");
+
+ _maxRetry = Integer.parseInt(_serverConfigs.getProperty(MAX_RETRY));
+ _imdsEndpoint = _serverConfigs.getProperty(IMDS_ENDPOINT);
+ int connectionTimeout = Integer.parseInt(_serverConfigs.getProperty(CONNECTION_TIMEOUT));
+ int requestTimeout = Integer.parseInt(_serverConfigs.getProperty(REQUEST_TIMEOUT));
+
+ final RequestConfig requestConfig = RequestConfig.custom()
+ .setConnectTimeout(connectionTimeout)
+ .setConnectionRequestTimeout(requestTimeout)
+ .build();
+
+ final HttpRequestRetryHandler httpRequestRetryHandler = (iOException, executionCount, httpContext) ->
+ !(executionCount >= _maxRetry
+ || iOException instanceof InterruptedIOException
+ || iOException instanceof UnknownHostException
+ || iOException instanceof SSLException
+ || HttpClientContext.adapt(httpContext).getRequest() instanceof HttpEntityEnclosingRequest);
+
+ _closeableHttpClient =
+ HttpClients.custom().setDefaultRequestConfig(requestConfig).setRetryHandler(httpRequestRetryHandler).build();
+ }
+
+ // Constructor for test purposes.
+ public AzureEnvironmentProvider(int maxRetry, String imdsEndpoint, CloseableHttpClient closeableHttpClient) {
+ Preconditions.checkArgument(maxRetry > 0, "maxRetry cannot be less than or equal to 0");
Review comment:
We don't need pre-conditions if we don't use this ctor for anything other than testing, 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] desaijay230592 commented on a change in pull request #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
desaijay230592 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r626956355
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -117,13 +125,18 @@
private AdminApiApplication _adminApiApplication;
private ServerQueriesDisabledTracker _serverQueriesDisabledTracker;
private RealtimeLuceneIndexRefreshState _realtimeLuceneIndexRefreshState;
+ private boolean skipPinotEnvironmentProviderFactoryInit = true;
- public HelixServerStarter(String helixClusterName, String zkAddress, PinotConfiguration serverConf)
- throws Exception {
+ public HelixServerStarter(String helixClusterName, String zkAddress, PinotConfiguration serverConf) throws Exception {
_helixClusterName = helixClusterName;
_zkAddress = zkAddress;
// Make a clone so that changes to the config won't propagate to the caller
_serverConf = serverConf.clone();
+ // Check if PinotEnvironmentProviderFactoryInit is enabled or not
+ if (_serverConf.containsKey(CONFIG_OF_ENABLE_ENVIRONMENT_PROVIDER_FACTORY) &&
+ TRUE.equals(serverConf.getProperty(CONFIG_OF_ENABLE_ENVIRONMENT_PROVIDER_FACTORY))) {
+ skipPinotEnvironmentProviderFactoryInit = false;
+ }
Review comment:
Moved the logic to the constructor.
--
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] xiangfu0 commented on a change in pull request #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
xiangfu0 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r627739876
##########
File path: pinot-plugins/pinot-environment/pinot-azure/pom.xml
##########
@@ -0,0 +1,65 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ 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.
+
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <artifactId>pinot-environment</artifactId>
+ <groupId>org.apache.pinot</groupId>
+ <version>0.8.0-SNAPSHOT</version>
+ <relativePath>..</relativePath>
+ </parent>
+ <artifactId>pinot-azure</artifactId>
+ <name>Pinot Azure Environment</name>
+ <url>https://pinot.apache.org/</url>
+ <properties>
+ <pinot.root>${basedir}/../../..</pinot.root>
+ <phase.prop>package</phase.prop>
+ </properties>
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-spi</artifactId>
+ <version>0.8.0-SNAPSHOT</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-common</artifactId>
+ <version>0.8.0-SNAPSHOT</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-core</artifactId>
+ <version>2.10.0</version>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.testng</groupId>
Review comment:
I think this is already declared in pinot-plugins pom.xml file
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
desaijay230592 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r631463213
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProviderFactory.java
##########
@@ -0,0 +1,84 @@
+/**
+ * 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.environmentprovider;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Iterables;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.plugin.PluginManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This factory class initializes the PinotEnvironmentProvider class.
+ * It creates a PinotEnvironment object based on the URI found.
+ */
+public class PinotEnvironmentProviderFactory {
+
+ private PinotEnvironmentProviderFactory() {
+ }
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(PinotEnvironmentProviderFactory.class);
+ private static final String CLASS = "class";
+ private static final Map<String, PinotEnvironmentProvider> ENVIRONMENT_TO_ENVIRONMENT_PROVIDER_MAP = new HashMap<>();
Review comment:
No we don't. An instance will only map to a single env provider. Map is used for easy lookup. Map of string -> list<provider> would have raised that question. Not sure why using Map of string -> provider would be a concern.
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r630652831
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -81,6 +83,8 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import static org.apache.pinot.spi.utils.CommonConstants.*;
Review comment:
Avoid static import
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -244,6 +300,10 @@ private void updateInstanceConfigIfNeeded(String host, int port) {
needToUpdateInstanceConfig = true;
}
+ // Update instance configs with environment specific properties
+ instanceConfig.getRecord().setMapField(ENVIRONMENT_IDENTIFIER, _environmentProperties);
Review comment:
Compare the existing map in instance config with the current map, and update when they are different (also need to set `needToUpdateInstanceConfig` to `true`
##########
File path: pinot-plugins/pinot-environment/pinot-azure/src/main/java/org/apache/pinot/plugin/provider/AzureEnvironmentProvider.java
##########
@@ -0,0 +1,172 @@
+/**
+ * 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.plugin.provider;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
+import java.util.HashMap;
+import java.util.Map;
+import javax.net.ssl.SSLException;
+import org.apache.commons.lang.StringUtils;
+import org.apache.http.HttpEntityEnclosingRequest;
+import org.apache.http.HttpStatus;
+import org.apache.http.StatusLine;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.protocol.HttpClientContext;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
+import org.apache.pinot.spi.utils.CommonConstants;
+
+
+/**
+ * Azure Environment Provider used to retrieve azure cloud specific instance configuration.
+ */
+public class AzureEnvironmentProvider implements PinotEnvironmentProvider {
+ 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";
+ private static final String COMPUTE = "compute";
+ private static final String METADATA = "Metadata";
+ private static final String PLATFORM_FAULT_DOMAIN = "platformFaultDomain";
+ private int _maxRetry;
+ private String _imdsEndpoint;
+ private CloseableHttpClient _closeableHttpClient;
+ private PinotConfiguration _serverConfigs;
+
+ public AzureEnvironmentProvider() {
+ }
+
+ public void init(PinotConfiguration pinotConfiguration) {
+ _serverConfigs = pinotConfiguration;
+ Preconditions.checkArgument(0 < Integer.parseInt(_serverConfigs.getProperty(MAX_RETRY)),
+ "[AzureEnvironmentProvider]: " + MAX_RETRY + " cannot be less than or equal to 0");
+ Preconditions.checkArgument(!StringUtils.isBlank(_serverConfigs.getProperty(IMDS_ENDPOINT)),
+ "[AzureEnvironmentProvider]: " + IMDS_ENDPOINT + " should not be null or empty");
+
+ _maxRetry = Integer.parseInt(_serverConfigs.getProperty(MAX_RETRY));
+ _imdsEndpoint = _serverConfigs.getProperty(IMDS_ENDPOINT);
+ int connectionTimeout = Integer.parseInt(_serverConfigs.getProperty(CONNECTION_TIMEOUT));
+ int requestTimeout = Integer.parseInt(_serverConfigs.getProperty(REQUEST_TIMEOUT));
+
+ final RequestConfig requestConfig = RequestConfig.custom()
+ .setConnectTimeout(connectionTimeout)
+ .setConnectionRequestTimeout(requestTimeout)
+ .build();
+
+ final HttpRequestRetryHandler httpRequestRetryHandler = (iOException, executionCount, httpContext) ->
+ !(executionCount >= _maxRetry
+ || iOException instanceof InterruptedIOException
+ || iOException instanceof UnknownHostException
+ || iOException instanceof SSLException
+ || HttpClientContext.adapt(httpContext).getRequest() instanceof HttpEntityEnclosingRequest);
+
+ _closeableHttpClient =
+ HttpClients.custom().setDefaultRequestConfig(requestConfig).setRetryHandler(httpRequestRetryHandler).build();
+ }
+
+ // Constructor for test purposes.
+ @VisibleForTesting
+ public AzureEnvironmentProvider(int maxRetry, String imdsEndpoint, CloseableHttpClient closeableHttpClient) {
+ _maxRetry = maxRetry;
+ _imdsEndpoint = imdsEndpoint;
+ _closeableHttpClient = Preconditions.checkNotNull(closeableHttpClient,
+ "[AzureEnvironmentProvider]: Closeable Http Client cannot be null");
+ }
+
+ /**
+ *
+ * Method for constructing custom pinot configuration map used by the HelixServerStarter to update
+ * zookeeper node with custom instance configs.
+ * @return custom pinot configuration map
+ */
+ @Override
+ public Map<String, String> getEnvironment() {
+ Map<String, String> customPinotConfiguration = new HashMap<>();
+
+ // Populate failure domain information
+ customPinotConfiguration.put(CommonConstants.INSTANCE_FAILURE_DOMAIN, getFailureDomain());
Review comment:
Directly put `FAILURE_DOMAIN_IDENTIFIER` here to avoid the extra key replacement
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -154,6 +164,52 @@ public HelixServerStarter(String helixClusterName, String zkAddress, PinotConfig
Server.DEFAULT_CURRENT_DATA_TABLE_VERSION));
}
+ /**
+ * Invoke pinot environment provider factory's init method to register the environment provider &
+ * fetch the overridden server configs for the invoked environment provider.
+ */
+ private void fetchEnvironmentSpecificConfigs() {
+ PinotConfiguration environmentProviderConfigs = _serverConf.subset(
+ Server.PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY);
+
+ if (environmentProviderConfigs.toMap().isEmpty()) {
+ LOGGER.info("No environment provider config values provided for server property: {}",
+ Server.PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY);
+ return;
+ }
+
+ PinotEnvironmentProviderFactory.init(environmentProviderConfigs);
+ String environmentProviderClassName = _serverConf.getProperty(Server.ENVIRONMENT_PROVIDER_CLASS_NAME);
+ if (environmentProviderClassName == null) {
+ LOGGER.info("No className value provided for property: {}", Server.ENVIRONMENT_PROVIDER_CLASS_NAME);
+ return;
+ }
+
+ // Fetch environment provider instance
+ PinotEnvironmentProvider pinotEnvironmentProvider = PinotEnvironmentProviderFactory.getEnvironmentProvider(
+ environmentProviderClassName.toLowerCase());
+
+ // Fetch the overridden pinot configuration containing custom configs.
+ Map<String, String> overriddenPinotConfigs = pinotEnvironmentProvider.getEnvironment();
Review comment:
```suggestion
@Nullable
private void fetchEnvironmentProperties() {
PinotConfiguration environmentProviderConfigs = _serverConf.subset(
Server.PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY);
if (environmentProviderConfigs.toMap().isEmpty()) {
LOGGER.info("No environment provider config values provided for server property: {}",
Server.PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY);
return null;
}
PinotEnvironmentProviderFactory.init(environmentProviderConfigs);
String environmentProviderClassName = _serverConf.getProperty(Server.ENVIRONMENT_PROVIDER_CLASS_NAME);
if (environmentProviderClassName == null) {
LOGGER.info("No className value provided for property: {}", Server.ENVIRONMENT_PROVIDER_CLASS_NAME);
return null;
}
// Fetch environment provider instance
PinotEnvironmentProvider pinotEnvironmentProvider = PinotEnvironmentProviderFactory.getEnvironmentProvider(
environmentProviderClassName.toLowerCase());
return pinotEnvironmentProvider.getEnvironment();
```
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -143,6 +148,11 @@ public HelixServerStarter(String helixClusterName, String zkAddress, PinotConfig
new HelixConfigScopeBuilder(ConfigScopeProperty.PARTICIPANT, _helixClusterName).forParticipant(_instanceId)
.build();
+ _environmentProperties = new HashMap<>();
+
+ // Fetch Environment Specific Configs
+ fetchEnvironmentSpecificConfigs();
Review comment:
Change `fetchEnvironmentSpecificConfigs()` to return a map or `null`
```suggestion
_environmentProperties = fetchEnvironmentProperties();
```
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProvider.java
##########
@@ -0,0 +1,40 @@
+/**
+ * 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.environmentprovider;
+
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+/**
+ * Environment Provider interface implemented by different cloud providers to customize
+ * the base pinot configuration to add environment variables & instance specific configuration
+ */
+public interface PinotEnvironmentProvider {
+
+ /**
+ * Customize base pinot configuration to add environment variables & instance specific configuration
+ * @return custom pinot configuration map
+ */
+ Map<String, String> getEnvironment();
+
+ /**
+ * Initializes the configurations specific to an environment provider.
+ */
+ void init(PinotConfiguration pinotConfiguration);
Review comment:
(nit) Move `init()` in front of `getEnvironment()`
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r631465800
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProviderFactory.java
##########
@@ -0,0 +1,84 @@
+/**
+ * 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.environmentprovider;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Iterables;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.plugin.PluginManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This factory class initializes the PinotEnvironmentProvider class.
+ * It creates a PinotEnvironment object based on the URI found.
+ */
+public class PinotEnvironmentProviderFactory {
+
+ private PinotEnvironmentProviderFactory() {
+ }
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(PinotEnvironmentProviderFactory.class);
+ private static final String CLASS = "class";
+ private static final Map<String, PinotEnvironmentProvider> ENVIRONMENT_TO_ENVIRONMENT_PROVIDER_MAP = new HashMap<>();
Review comment:
So, if there is ony one entry in the map, why not just use a single string variable instead of a map?
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#issuecomment-834835328
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6842?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 [#6842](https://codecov.io/gh/apache/incubator-pinot/pull/6842?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4f522ab) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/f0648115b34e23c5eff9a3e86748e09b3d3d04e8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f064811) will **decrease** coverage by `0.03%`.
> The diff coverage is `29.50%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6842?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 #6842 +/- ##
============================================
- Coverage 65.37% 65.34% -0.04%
Complexity 12 12
============================================
Files 1425 1426 +1
Lines 70240 70301 +61
Branches 10143 10150 +7
============================================
+ Hits 45920 45937 +17
- Misses 21038 21079 +41
- Partials 3282 3285 +3
```
| Flag | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| unittests | `65.34% <29.50%> (-0.04%)` | `12.00 <0.00> (ø)` | |
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/6842?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...pinot/server/starter/helix/HelixServerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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%> (ø)` | `0.00 <0.00> (ø)` | |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `31.48% <ø> (ø)` | `0.00 <0.00> (ø)` | |
| [...nmentprovider/PinotEnvironmentProviderFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZW52aXJvbm1lbnRwcm92aWRlci9QaW5vdEVudmlyb25tZW50UHJvdmlkZXJGYWN0b3J5LmphdmE=) | `64.28% <64.28%> (ø)` | `0.00 <0.00> (?)` | |
| [...ore/query/scheduler/resources/ResourceManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9zY2hlZHVsZXIvcmVzb3VyY2VzL1Jlc291cmNlTWFuYWdlci5qYXZh) | `85.71% <0.00%> (-10.72%)` | `0.00% <0.00%> (ø%)` | |
| [.../pinot/core/query/scheduler/PriorityScheduler.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9zY2hlZHVsZXIvUHJpb3JpdHlTY2hlZHVsZXIuamF2YQ==) | `78.08% <0.00%> (-2.74%)` | `0.00% <0.00%> (ø%)` | |
| [.../java/org/apache/pinot/spi/data/TimeFieldSpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9UaW1lRmllbGRTcGVjLmphdmE=) | `88.63% <0.00%> (-2.28%)` | `0.00% <0.00%> (ø%)` | |
| [...lix/core/realtime/PinotRealtimeSegmentManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/6842/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYWx0aW1lL1Bpbm90UmVhbHRpbWVTZWdtZW50TWFuYWdlci5qYXZh) | `78.46% <0.00%> (+2.56%)` | `0.00% <0.00%> (ø%)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6842?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/6842?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 [f064811...4f522ab](https://codecov.io/gh/apache/incubator-pinot/pull/6842?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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
desaijay230592 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r620753371
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -20,9 +20,10 @@
import java.io.File;
-
public class CommonConstants {
+ public static final String PLATFORM_FAULT_DOMAIN = "platformFaultDomain";
Review comment:
Updated.
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
desaijay230592 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r630463840
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -261,6 +306,22 @@ private void updateInstanceConfigIfNeeded(String host, int port) {
"Failed to update instance config");
}
+ // Update instance configs with environment specific properties
+ private void updateInstanceConfigWithEnvironmentSpecificConfigs() {
Review comment:
After proper consideration it seemed best to update instance configs at Server Starter layer. We are already doing something similar. Refer to: updateInstanceConfigIfNeeded method in HelixServerStarter.java. Updating configs before the call to HelixServerStarter is not possible as we initialize the EnvironmentProvider factory at HelixServerStarter layer. If we move the entire logic before the call to HelixServerStarter is made then we will be adding a lot of redundant code of connecting to helixManager and retrieving the helixAdmin instance. Hence, this solution.
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r630403340
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -261,6 +306,22 @@ private void updateInstanceConfigIfNeeded(String host, int port) {
"Failed to update instance config");
}
+ // Update instance configs with environment specific properties
+ private void updateInstanceConfigWithEnvironmentSpecificConfigs() {
Review comment:
Didn't we conclude in the design document that the instance configs will be set before HelixServerStarter is called?
--
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] xiangfu0 commented on a change in pull request #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
xiangfu0 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r627738597
##########
File path: pinot-plugins/pinot-environment/pinot-azure/pom.xml
##########
@@ -0,0 +1,65 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ 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.
+
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <artifactId>pinot-environment</artifactId>
+ <groupId>org.apache.pinot</groupId>
+ <version>0.8.0-SNAPSHOT</version>
+ <relativePath>..</relativePath>
+ </parent>
+ <artifactId>pinot-azure</artifactId>
+ <name>Pinot Azure Environment</name>
+ <url>https://pinot.apache.org/</url>
+ <properties>
+ <pinot.root>${basedir}/../../..</pinot.root>
+ <phase.prop>package</phase.prop>
+ </properties>
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-spi</artifactId>
+ <version>0.8.0-SNAPSHOT</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-common</artifactId>
+ <version>0.8.0-SNAPSHOT</version>
Review comment:
<version>${project.version}</version>
##########
File path: pinot-plugins/pinot-environment/pinot-azure/pom.xml
##########
@@ -0,0 +1,65 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ 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.
+
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <artifactId>pinot-environment</artifactId>
+ <groupId>org.apache.pinot</groupId>
+ <version>0.8.0-SNAPSHOT</version>
+ <relativePath>..</relativePath>
+ </parent>
+ <artifactId>pinot-azure</artifactId>
+ <name>Pinot Azure Environment</name>
+ <url>https://pinot.apache.org/</url>
+ <properties>
+ <pinot.root>${basedir}/../../..</pinot.root>
+ <phase.prop>package</phase.prop>
+ </properties>
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-spi</artifactId>
+ <version>0.8.0-SNAPSHOT</version>
Review comment:
<version>${project.version}</version>
--
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] rkanumul commented on a change in pull request #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
rkanumul commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r621504610
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -103,9 +107,13 @@
public class HelixServerStarter implements ServiceStartable {
private static final Logger LOGGER = LoggerFactory.getLogger(HelixServerStarter.class);
+ private static final String ENVIRONMENT_IDENTIFIER = "environment";
+ private static final String FAILURE_DOMAIN_IDENTIFIER = "failureDomain";
+ private static final String TRUE = "true";
Review comment:
Boolean.TRUE.toString()
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -321,6 +357,16 @@ public void start()
LOGGER.info("Initializing server instance and registering state model factory");
Utils.logVersions();
ControllerLeaderLocator.create(_helixManager);
+
+ // Invoke pinot environment provider factory's init method to register the environment provider
+ // and fetch the overridden server configs for the invoked environment provider
+ if (!skipPinotEnvironmentProviderFactoryInit) {
+ PinotEnvironmentProviderFactory.init(_serverConf.subset(PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY));
+ PinotEnvironmentProvider pinotEnvironmentProvider = PinotEnvironmentProviderFactory.getEnvironmentProvider(
+ _serverConf.getProperty(CONFIG_OF_ENVIRONMENT_PROVIDER_TYPE));
+ _overriddenServerConfigs = pinotEnvironmentProvider.getEnvironment(_serverConf.toMap());
Review comment:
why have overridden configs different from server config ?
it is "overidden" with the values appropriate to environment.
this is the new server config now.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -321,6 +321,11 @@
public static final String CONFIG_OF_ENABLE_THREAD_CPU_TIME_MEASUREMENT =
"pinot.server.instance.enableThreadCpuTimeMeasurement";
public static final boolean DEFAULT_ENABLE_THREAD_CPU_TIME_MEASUREMENT = false;
+
+ // Environment Provider Configs
+ public static final String PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY = "pinot.server.environmentProvider.factory";
+ public static final String CONFIG_OF_ENABLE_ENVIRONMENT_PROVIDER_FACTORY = "pinot.server.environmentProvider.enabled";
+ public static final String CONFIG_OF_ENVIRONMENT_PROVIDER_TYPE = "pinot.server.environmentProvider.type";
Review comment:
what is type ?
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -117,13 +125,18 @@
private AdminApiApplication _adminApiApplication;
private ServerQueriesDisabledTracker _serverQueriesDisabledTracker;
private RealtimeLuceneIndexRefreshState _realtimeLuceneIndexRefreshState;
+ private boolean skipPinotEnvironmentProviderFactoryInit = true;
- public HelixServerStarter(String helixClusterName, String zkAddress, PinotConfiguration serverConf)
- throws Exception {
+ public HelixServerStarter(String helixClusterName, String zkAddress, PinotConfiguration serverConf) throws Exception {
_helixClusterName = helixClusterName;
_zkAddress = zkAddress;
// Make a clone so that changes to the config won't propagate to the caller
_serverConf = serverConf.clone();
+ // Check if PinotEnvironmentProviderFactoryInit is enabled or not
+ if (_serverConf.containsKey(CONFIG_OF_ENABLE_ENVIRONMENT_PROVIDER_FACTORY) &&
+ TRUE.equals(serverConf.getProperty(CONFIG_OF_ENABLE_ENVIRONMENT_PROVIDER_FACTORY))) {
+ skipPinotEnvironmentProviderFactoryInit = false;
+ }
Review comment:
this check can go directly to wherever you are using the variable. no need to have this additional variable.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -321,6 +321,11 @@
public static final String CONFIG_OF_ENABLE_THREAD_CPU_TIME_MEASUREMENT =
"pinot.server.instance.enableThreadCpuTimeMeasurement";
public static final boolean DEFAULT_ENABLE_THREAD_CPU_TIME_MEASUREMENT = false;
+
+ // Environment Provider Configs
+ public static final String PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY = "pinot.server.environmentProvider.factory";
+ public static final String CONFIG_OF_ENABLE_ENVIRONMENT_PROVIDER_FACTORY = "pinot.server.environmentProvider.enabled";
Review comment:
do we need this ? is not having an env provider same as "false" ?
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -103,9 +107,13 @@
public class HelixServerStarter implements ServiceStartable {
private static final Logger LOGGER = LoggerFactory.getLogger(HelixServerStarter.class);
+ private static final String ENVIRONMENT_IDENTIFIER = "environment";
+ private static final String FAILURE_DOMAIN_IDENTIFIER = "failureDomain";
Review comment:
these can be in common constants ? given this config can be populated by other components as well ?
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -117,13 +125,18 @@
private AdminApiApplication _adminApiApplication;
private ServerQueriesDisabledTracker _serverQueriesDisabledTracker;
private RealtimeLuceneIndexRefreshState _realtimeLuceneIndexRefreshState;
+ private boolean skipPinotEnvironmentProviderFactoryInit = true;
- public HelixServerStarter(String helixClusterName, String zkAddress, PinotConfiguration serverConf)
- throws Exception {
+ public HelixServerStarter(String helixClusterName, String zkAddress, PinotConfiguration serverConf) throws Exception {
_helixClusterName = helixClusterName;
_zkAddress = zkAddress;
// Make a clone so that changes to the config won't propagate to the caller
_serverConf = serverConf.clone();
+ // Check if PinotEnvironmentProviderFactoryInit is enabled or not
+ if (_serverConf.containsKey(CONFIG_OF_ENABLE_ENVIRONMENT_PROVIDER_FACTORY) &&
+ TRUE.equals(serverConf.getProperty(CONFIG_OF_ENABLE_ENVIRONMENT_PROVIDER_FACTORY))) {
+ skipPinotEnvironmentProviderFactoryInit = false;
+ }
Review comment:
can we not get the overridden configs here before anything happens at all ?
that way we can make the overidden configs as the new set of configs for the server
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
desaijay230592 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r626956411
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -321,6 +357,16 @@ public void start()
LOGGER.info("Initializing server instance and registering state model factory");
Utils.logVersions();
ControllerLeaderLocator.create(_helixManager);
+
+ // Invoke pinot environment provider factory's init method to register the environment provider
+ // and fetch the overridden server configs for the invoked environment provider
+ if (!skipPinotEnvironmentProviderFactoryInit) {
+ PinotEnvironmentProviderFactory.init(_serverConf.subset(PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY));
+ PinotEnvironmentProvider pinotEnvironmentProvider = PinotEnvironmentProviderFactory.getEnvironmentProvider(
+ _serverConf.getProperty(CONFIG_OF_ENVIRONMENT_PROVIDER_TYPE));
+ _overriddenServerConfigs = pinotEnvironmentProvider.getEnvironment(_serverConf.toMap());
Review comment:
Done.
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -117,13 +125,18 @@
private AdminApiApplication _adminApiApplication;
private ServerQueriesDisabledTracker _serverQueriesDisabledTracker;
private RealtimeLuceneIndexRefreshState _realtimeLuceneIndexRefreshState;
+ private boolean skipPinotEnvironmentProviderFactoryInit = true;
- public HelixServerStarter(String helixClusterName, String zkAddress, PinotConfiguration serverConf)
- throws Exception {
+ public HelixServerStarter(String helixClusterName, String zkAddress, PinotConfiguration serverConf) throws Exception {
_helixClusterName = helixClusterName;
_zkAddress = zkAddress;
// Make a clone so that changes to the config won't propagate to the caller
_serverConf = serverConf.clone();
+ // Check if PinotEnvironmentProviderFactoryInit is enabled or not
+ if (_serverConf.containsKey(CONFIG_OF_ENABLE_ENVIRONMENT_PROVIDER_FACTORY) &&
+ TRUE.equals(serverConf.getProperty(CONFIG_OF_ENABLE_ENVIRONMENT_PROVIDER_FACTORY))) {
+ skipPinotEnvironmentProviderFactoryInit = false;
+ }
Review comment:
Done.
--
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] rkanumul commented on a change in pull request #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
rkanumul commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r620585103
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -321,6 +323,10 @@ public void start()
LOGGER.info("Initializing server instance and registering state model factory");
Utils.logVersions();
ControllerLeaderLocator.create(_helixManager);
+
+ // Invoke Pinot Environment Provider's Init method to register the environment provider
+ PinotEnvironmentProviderFactory.init(_serverConf.subset(PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY));
Review comment:
Also what happens to existing setups with no provider. we should skip in case someone is not interested in setting up the provider.
--
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] rkanumul commented on a change in pull request #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
rkanumul commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r620583918
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -321,6 +323,10 @@ public void start()
LOGGER.info("Initializing server instance and registering state model factory");
Utils.logVersions();
ControllerLeaderLocator.create(_helixManager);
+
+ // Invoke Pinot Environment Provider's Init method to register the environment provider
+ PinotEnvironmentProviderFactory.init(_serverConf.subset(PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY));
Review comment:
This inits the provider. can we also do getEnvironment to override the configs ?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -20,9 +20,10 @@
import java.io.File;
-
public class CommonConstants {
+ public static final String PLATFORM_FAULT_DOMAIN = "platformFaultDomain";
Review comment:
This constant can be in AzureEnvironmentProvider
given it is a constant tightly related to azure
--
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] rkanumul commented on a change in pull request #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
rkanumul commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r627758592
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -119,51 +122,57 @@
private ServerQueriesDisabledTracker _serverQueriesDisabledTracker;
private RealtimeLuceneIndexRefreshState _realtimeLuceneIndexRefreshState;
- public HelixServerStarter(String helixClusterName, String zkAddress, PinotConfiguration serverConf)
- throws Exception {
+ public HelixServerStarter(String helixClusterName, String zkAddress, PinotConfiguration serverConf) throws Exception {
_helixClusterName = helixClusterName;
_zkAddress = zkAddress;
// Make a clone so that changes to the config won't propagate to the caller
_serverConf = serverConf.clone();
_listenerConfigs = ListenerConfigUtil.buildServerAdminConfigs(_serverConf);
- _host = _serverConf.getProperty(Helix.KEY_OF_SERVER_NETTY_HOST,
- _serverConf.getProperty(Helix.SET_INSTANCE_ID_TO_HOSTNAME_KEY, false) ? NetUtils.getHostnameOrAddress()
+ _host = _serverConf.getProperty(KEY_OF_SERVER_NETTY_HOST,
+ _serverConf.getProperty(SET_INSTANCE_ID_TO_HOSTNAME_KEY, false) ? NetUtils.getHostnameOrAddress()
: NetUtils.getHostAddress());
- _port = _serverConf.getProperty(Helix.KEY_OF_SERVER_NETTY_PORT, Helix.DEFAULT_SERVER_NETTY_PORT);
+ _port = _serverConf.getProperty(KEY_OF_SERVER_NETTY_PORT, DEFAULT_SERVER_NETTY_PORT);
- String instanceId = _serverConf.getProperty(Server.CONFIG_OF_INSTANCE_ID);
+ String instanceId = _serverConf.getProperty(CONFIG_OF_INSTANCE_ID);
if (instanceId == null) {
- instanceId = Helix.PREFIX_OF_SERVER_INSTANCE + _host + "_" + _port;
- _serverConf.addProperty(Server.CONFIG_OF_INSTANCE_ID, instanceId);
+ instanceId = PREFIX_OF_SERVER_INSTANCE + _host + "_" + _port;
+ _serverConf.addProperty(CONFIG_OF_INSTANCE_ID, instanceId);
}
_instanceId = instanceId;
_instanceConfigScope =
new HelixConfigScopeBuilder(ConfigScopeProperty.PARTICIPANT, _helixClusterName).forParticipant(_instanceId)
.build();
+ // Invoke pinot environment provider factory's init method to register the environment provider
+ for (String key : _serverConf.toMap().keySet()) {
+ if (key.startsWith(PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY.toLowerCase())) {
Review comment:
why not get it from the map instead of iterating ? that is more efficient
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -261,12 +270,39 @@ private void updateInstanceConfigIfNeeded(String host, int port) {
"Failed to update instance config");
}
+ // Fetch the overridden server configs for the invoked environment provider
+ private void populateFailureDomain() {
+ String className = _serverConf.getProperty(ENVIRONMENT_PROVIDER_CLASS_NAME);
+ if (className == null) return;
Review comment:
some comments and spacing b/w blocks to break logic will make it more readable.
This function has lot of logic, the comments will help.
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -261,12 +270,39 @@ private void updateInstanceConfigIfNeeded(String host, int port) {
"Failed to update instance config");
}
+ // Fetch the overridden server configs for the invoked environment provider
+ private void populateFailureDomain() {
+ String className = _serverConf.getProperty(ENVIRONMENT_PROVIDER_CLASS_NAME);
+ if (className == null) return;
+ PinotEnvironmentProvider pinotEnvironmentProvider = PinotEnvironmentProviderFactory.getEnvironmentProvider(className.toLowerCase());
+ _serverConf = pinotEnvironmentProvider.getEnvironment(_serverConf.toMap());
+ Map<String, Object> overriddenPinotConfigurationMap = _serverConf.toMap();
Review comment:
essentially this part to be in constructor
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -103,10 +107,9 @@
*/
public class HelixServerStarter implements ServiceStartable {
private static final Logger LOGGER = LoggerFactory.getLogger(HelixServerStarter.class);
-
private final String _helixClusterName;
private final String _zkAddress;
- private final PinotConfiguration _serverConf;
+ private PinotConfiguration _serverConf;
Review comment:
If we override the configs in the constructor, we can keep this as final.
can we not do that ?
populating the instance config can happen on startup.
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProviderFactory.java
##########
@@ -0,0 +1,82 @@
+/**
+ * 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.environmentprovider;
+
+import com.google.common.base.Preconditions;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.plugin.PluginManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This factory class initializes the PinotEnvironmentProvider class.
+ * It creates a PinotEnvironment object based on the URI found.
+ */
+public class PinotEnvironmentProviderFactory {
+
+ private PinotEnvironmentProviderFactory() {
+ }
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(PinotEnvironmentProviderFactory.class);
+ private static final String CLASS = "class";
+ private static final Map<String, PinotEnvironmentProvider> PINOT_ENVIRONMENT_PROVIDER_MAP = new HashMap<>();
+
+ public static void register(
+ String environment, String environmentProviderFileName, PinotConfiguration environmentProviderConfiguration) {
+ try {
+ LOGGER.info("Initializing PinotEnvironmentProvider for environment {}, classname {}", environment, environmentProviderFileName);
+ PinotEnvironmentProvider pinotEnvironmentProvider = PluginManager.get().createInstance(environmentProviderFileName);
+ pinotEnvironmentProvider.init(environmentProviderConfiguration);
+ PINOT_ENVIRONMENT_PROVIDER_MAP.put(environment, pinotEnvironmentProvider);
+ } catch (Exception ex) {
+ LOGGER.error(
+ "Could not instantiate environment provider for class {} with environment {}", environmentProviderFileName, environment, ex);
+ throw new RuntimeException(ex);
+ }
+ }
+
+ public static void init(PinotConfiguration environmentProviderFactoryConfig) {
+ // Get environment and their respective classes
+ PinotConfiguration environmentConfiguration = environmentProviderFactoryConfig.subset(CLASS);
+ List<String> environments = environmentConfiguration.getKeys();
+
+ if (environments.isEmpty()) {
+ LOGGER.info("Did not find any environment provider classes in the configuration");
+ return;
+ }
+
+ for (String environment : environments) {
Review comment:
we dont have multiple environments at once ? based on how the config is, we can only setup one. Isn't that true?
An example of how the config looks will help
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -66,21 +66,25 @@
import org.apache.pinot.server.starter.ServerInstance;
import org.apache.pinot.server.starter.ServerQueriesDisabledTracker;
import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
+import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProviderFactory;
import org.apache.pinot.spi.filesystem.PinotFSFactory;
import org.apache.pinot.spi.plugin.PluginManager;
import org.apache.pinot.spi.services.ServiceRole;
import org.apache.pinot.spi.services.ServiceStartable;
import org.apache.pinot.spi.utils.CommonConstants;
-import org.apache.pinot.spi.utils.CommonConstants.Helix;
import org.apache.pinot.spi.utils.CommonConstants.Helix.Instance;
import org.apache.pinot.spi.utils.CommonConstants.Helix.StateModel;
-import org.apache.pinot.spi.utils.CommonConstants.Server;
import org.apache.pinot.spi.utils.CommonConstants.Server.SegmentCompletionProtocol;
import org.apache.pinot.spi.utils.NetUtils;
import org.apache.pinot.spi.utils.builder.TableNameBuilder;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import static org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider.*;
+import static org.apache.pinot.spi.utils.CommonConstants.*;
+import static org.apache.pinot.spi.utils.CommonConstants.Helix.*;
+import static org.apache.pinot.spi.utils.CommonConstants.Server.*;
Review comment:
Let's stick to class imports and refer constants as classname.constant
--
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] xiangfu0 merged pull request #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
xiangfu0 merged pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r630577925
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -665,4 +731,4 @@ public static void main(String[] args)
throws Exception {
startDefault();
}
-}
+}
Review comment:
Revert
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -118,6 +120,7 @@
private AdminApiApplication _adminApiApplication;
private ServerQueriesDisabledTracker _serverQueriesDisabledTracker;
private RealtimeLuceneIndexRefreshState _realtimeLuceneIndexRefreshState;
+ private static Map<String, Map<String, String>> environmentSpecificMapFields = new HashMap<>();
Review comment:
This should not be static.
We don't need to keep a map of map
```suggestion
private Map<String, String> environmentProperties;
```
##########
File path: pinot-plugins/pinot-environment/pinot-azure/src/main/java/org/apache/pinot/plugin/provider/AzureEnvironmentProvider.java
##########
@@ -0,0 +1,172 @@
+/**
+ * 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.plugin.provider;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
+import java.util.HashMap;
+import java.util.Map;
+import javax.net.ssl.SSLException;
+import org.apache.commons.lang.StringUtils;
+import org.apache.http.HttpEntityEnclosingRequest;
+import org.apache.http.HttpStatus;
+import org.apache.http.StatusLine;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.protocol.HttpClientContext;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
+import org.apache.pinot.spi.utils.CommonConstants;
+
+
+/**
+ * Azure Environment Provider used to retrieve azure cloud specific instance configuration.
+ */
+public class AzureEnvironmentProvider implements PinotEnvironmentProvider {
+ 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";
+ private static final String COMPUTE = "compute";
+ private static final String METADATA = "Metadata";
+ private static final String PLATFORM_FAULT_DOMAIN = "platformFaultDomain";
+ private int _maxRetry;
+ private String _imdsEndpoint;
+ private CloseableHttpClient _closeableHttpClient;
+ private PinotConfiguration _serverConfigs;
+
+ public AzureEnvironmentProvider() {
+ }
+
+ public void init(PinotConfiguration pinotConfiguration) {
+ _serverConfigs = pinotConfiguration;
+ Preconditions.checkArgument(0 < Integer.parseInt(_serverConfigs.getProperty(MAX_RETRY)),
+ "[AzureEnvironmentProvider]: " + MAX_RETRY + " cannot be less than or equal to 0");
+ Preconditions.checkArgument(!StringUtils.isBlank(_serverConfigs.getProperty(IMDS_ENDPOINT)),
+ "[AzureEnvironmentProvider]: " + IMDS_ENDPOINT + " should not be null or empty");
+
+ _maxRetry = Integer.parseInt(_serverConfigs.getProperty(MAX_RETRY));
+ _imdsEndpoint = _serverConfigs.getProperty(IMDS_ENDPOINT);
+ int connectionTimeout = Integer.parseInt(_serverConfigs.getProperty(CONNECTION_TIMEOUT));
+ int requestTimeout = Integer.parseInt(_serverConfigs.getProperty(REQUEST_TIMEOUT));
+
+ final RequestConfig requestConfig = RequestConfig.custom()
+ .setConnectTimeout(connectionTimeout)
+ .setConnectionRequestTimeout(requestTimeout)
+ .build();
+
+ final HttpRequestRetryHandler httpRequestRetryHandler = (iOException, executionCount, httpContext) ->
+ !(executionCount >= _maxRetry
+ || iOException instanceof InterruptedIOException
+ || iOException instanceof UnknownHostException
+ || iOException instanceof SSLException
+ || HttpClientContext.adapt(httpContext).getRequest() instanceof HttpEntityEnclosingRequest);
+
+ _closeableHttpClient =
+ HttpClients.custom().setDefaultRequestConfig(requestConfig).setRetryHandler(httpRequestRetryHandler).build();
+ }
+
+ // Constructor for test purposes.
+ @VisibleForTesting
+ public AzureEnvironmentProvider(int maxRetry, String imdsEndpoint, CloseableHttpClient closeableHttpClient) {
+ _maxRetry = maxRetry;
+ _imdsEndpoint = imdsEndpoint;
+ _closeableHttpClient = Preconditions.checkNotNull(closeableHttpClient,
+ "[AzureEnvironmentProvider]: Closeable Http Client cannot be null");
+ }
+
+ /**
+ *
+ * Method for constructing custom pinot configuration used by the HelixServerStarter to update
+ * zookeeper node with custom instance configs.
+ * @return custom pinot configuration map
+ */
+ @Override
+ public Map<String, Object> getEnvironment() {
+ Map<String, Object> customPinotConfiguration = new HashMap<>(_serverConfigs.toMap());
Review comment:
Any specific reason why we put all server configs into the environment? This would include configs such as `maxRetry`, `connectionTimeout` which are not related to environment.
If we only put properties that we want to store in helix, in the `HelixServerStarter` we can simply put the whole map as the environment. This way is more generic, and works if we add more properties in the future.
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -261,6 +308,22 @@ private void updateInstanceConfigIfNeeded(String host, int port) {
"Failed to update instance config");
}
+ // Update instance configs with environment specific properties
+ private void updateInstanceConfigWithEnvironmentSpecificConfigs() {
Review comment:
Suggest merging this into the `updateInstanceConfigIfNeeded`
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
desaijay230592 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r631309663
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProvider.java
##########
@@ -0,0 +1,40 @@
+/**
+ * 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.environmentprovider;
+
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+/**
+ * Environment Provider interface implemented by different cloud providers to customize
+ * the base pinot configuration to add environment variables & instance specific configuration
+ */
+public interface PinotEnvironmentProvider {
+
+ /**
+ * Initializes the configurations specific to an environment provider.
+ */
+ void init(PinotConfiguration pinotConfiguration);
+
+ /**
+ * Customize base pinot configuration to add environment variables & instance specific configuration
+ * @return custom pinot configuration map
+ */
+ Map<String, String> getEnvironment();
Review comment:
The plan is to add all the environment properties under mapFields. If u see the getEnvironment() method returns a Map. For now we are only adding FD information, but other cloud providers can add other environment properties. At HelixServerStarter layer we will update instanceConfig with the entire map returned by getEnvironment() call.
Currently, the only thing pinot can derive sense out of is the failure domain information. Pinot can use FD information to assign server to server pools to facilitate segment & replica group assignments.
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
desaijay230592 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r620730001
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -321,6 +323,10 @@ public void start()
LOGGER.info("Initializing server instance and registering state model factory");
Utils.logVersions();
ControllerLeaderLocator.create(_helixManager);
+
+ // Invoke Pinot Environment Provider's Init method to register the environment provider
+ PinotEnvironmentProviderFactory.init(_serverConf.subset(PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY));
Review comment:
Done.
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
desaijay230592 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r626956189
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -103,9 +107,13 @@
public class HelixServerStarter implements ServiceStartable {
private static final Logger LOGGER = LoggerFactory.getLogger(HelixServerStarter.class);
+ private static final String ENVIRONMENT_IDENTIFIER = "environment";
+ private static final String FAILURE_DOMAIN_IDENTIFIER = "failureDomain";
+ private static final String TRUE = "true";
Review comment:
Done.
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
desaijay230592 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r628467875
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProvider.java
##########
@@ -0,0 +1,43 @@
+/**
+ * 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.environmentprovider;
+
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+/**
+ * Environment Provider interface implemented by different cloud
+ * providers to customize the base pinot configuration to add environment
+ * variables & instance specific configuration
+ */
+public interface PinotEnvironmentProvider {
+
+ String INSTANCE_FAILURE_DOMAIN = "pinot.environment.instance.failureDomain";
+ /**
+ * Customize base pinot configuration to add environment variable & instance specific configuration
+ * @param baseConfiguration Base configuration provided by the invoking service
+ * @return Custom Pinot Configuration
+ */
+ PinotConfiguration getEnvironment(Map<String, Object> baseConfiguration);
Review comment:
Made the changes. Removed passing of instance config in two places.
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r631465800
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProviderFactory.java
##########
@@ -0,0 +1,84 @@
+/**
+ * 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.environmentprovider;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Iterables;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.plugin.PluginManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This factory class initializes the PinotEnvironmentProvider class.
+ * It creates a PinotEnvironment object based on the URI found.
+ */
+public class PinotEnvironmentProviderFactory {
+
+ private PinotEnvironmentProviderFactory() {
+ }
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(PinotEnvironmentProviderFactory.class);
+ private static final String CLASS = "class";
+ private static final Map<String, PinotEnvironmentProvider> ENVIRONMENT_TO_ENVIRONMENT_PROVIDER_MAP = new HashMap<>();
Review comment:
So, if there is ony one entry in the map, why not just use a single variable instead of a map?
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r627845646
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -261,12 +270,39 @@ private void updateInstanceConfigIfNeeded(String host, int port) {
"Failed to update instance config");
}
+ // Fetch the overridden server configs for the invoked environment provider
+ private void populateFailureDomain() {
+ String className = _serverConf.getProperty(ENVIRONMENT_PROVIDER_CLASS_NAME);
+ if (className == null) return;
+ PinotEnvironmentProvider pinotEnvironmentProvider = PinotEnvironmentProviderFactory.getEnvironmentProvider(className.toLowerCase());
+ _serverConf = pinotEnvironmentProvider.getEnvironment(_serverConf.toMap());
+ Map<String, Object> overriddenPinotConfigurationMap = _serverConf.toMap();
+ String failureDomain = overriddenPinotConfigurationMap.containsKey(INSTANCE_FAILURE_DOMAIN.toLowerCase()) ?
+ String.valueOf(overriddenPinotConfigurationMap.get(INSTANCE_FAILURE_DOMAIN.toLowerCase())) : null;
+ if (failureDomain == null) {
+ LOGGER.info("No failure domain information found for instance: {}", _instanceId);
+ return;
+ }
+ Map<String, String> failureDomainMap = new HashMap<>();
+ failureDomainMap.put(FAILURE_DOMAIN_IDENTIFIER, failureDomain);
Review comment:
How about other environment properties? We should put all of them here
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProvider.java
##########
@@ -0,0 +1,43 @@
+/**
+ * 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.environmentprovider;
+
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+/**
+ * Environment Provider interface implemented by different cloud
+ * providers to customize the base pinot configuration to add environment
+ * variables & instance specific configuration
+ */
+public interface PinotEnvironmentProvider {
+
+ String INSTANCE_FAILURE_DOMAIN = "pinot.environment.instance.failureDomain";
+ /**
+ * Customize base pinot configuration to add environment variable & instance specific configuration
+ * @param baseConfiguration Base configuration provided by the invoking service
+ * @return Custom Pinot Configuration
+ */
+ PinotConfiguration getEnvironment(Map<String, Object> baseConfiguration);
Review comment:
Why do we need to pass in the instance config twice (once in `init` and once `here`)? We should only pass it once.
Since the environment properties will be stored as a map field in the ZNRecord, we should probably just return a `Map<String, String>` here.
```suggestion
Map<String, String> getEnvironment();
```
--
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 pull request #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#issuecomment-839328424
> I still think it is better to keep this config in zookeeper rather than configuring each component instance separately, and hoping that they are all the same (scheme as per my proposal in the design document).
>
> @kishoreg , @daniellavoie comments?
We concluded that we can keep it local for now. It works the same as other plugins like PinotFs. We can implement a zk + merge-with-local later, where we can set some of the parameters via local configuration (e.g. secrets)
--
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 #6842: Core Pinot Environment Provider Implementation Logic to fetch Failure…
Posted by GitBox <gi...@apache.org>.
desaijay230592 commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r630464367
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProviderFactory.java
##########
@@ -0,0 +1,82 @@
+/**
+ * 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.environmentprovider;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Iterables;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.plugin.PluginManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This factory class initializes the PinotEnvironmentProvider class.
+ * It creates a PinotEnvironment object based on the URI found.
+ */
+public class PinotEnvironmentProviderFactory {
+
+ private PinotEnvironmentProviderFactory() {
+ }
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(PinotEnvironmentProviderFactory.class);
+ private static final String CLASS = "class";
+ private static final Map<String, PinotEnvironmentProvider> PINOT_ENVIRONMENT_PROVIDER_MAP = new HashMap<>();
+
+ public static void init(PinotConfiguration environmentProviderFactoryConfig) {
+ // Get environment and it's respective class
+ PinotConfiguration environmentConfiguration = environmentProviderFactoryConfig.subset(CLASS);
+ List<String> environments = environmentConfiguration.getKeys();
+
+ if (environments.isEmpty()) {
+ LOGGER.info("Did not find any environment provider classes in the configuration");
+ return;
+ }
+
+ String environment = Iterables.getOnlyElement(environments);
+ String environmentProviderClassName = environmentConfiguration.getProperty(environment);
+ PinotConfiguration environmentProviderConfiguration = environmentProviderFactoryConfig.subset(environment);
+ LOGGER.info("Got environment {}, initializing class {}", environment, environmentProviderClassName);
+ register(environment, environmentProviderClassName, environmentProviderConfiguration);
+ }
+
+ // Utility to invoke the cloud specific environment provider.
+ public static PinotEnvironmentProvider getEnvironmentProvider(String environment) {
+ PinotEnvironmentProvider pinotEnvironmentProvider = PINOT_ENVIRONMENT_PROVIDER_MAP.get(environment);
+ Preconditions.checkState(pinotEnvironmentProvider != null,
+ "PinotEnvironmentProvider for environment: %s has not been initialized", environment);
+ return pinotEnvironmentProvider;
+ }
+
+ private static void register(
+ String environment, String environmentProviderFileName, PinotConfiguration environmentProviderConfiguration) {
+ try {
+ LOGGER.info("Initializing PinotEnvironmentProvider for environment {}, classname {}", environment, environmentProviderFileName);
+ PinotEnvironmentProvider pinotEnvironmentProvider = PluginManager.get().createInstance(environmentProviderFileName);
+ pinotEnvironmentProvider.init(environmentProviderConfiguration);
+ PINOT_ENVIRONMENT_PROVIDER_MAP.put(environment, pinotEnvironmentProvider);
Review comment:
Added a check if the map already contains the environment key. We will populate the map only for the first registration request for an environment.
--
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