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/03/29 17:37:57 UTC
[GitHub] [incubator-pinot] jackjlli opened a new pull request #6725: Explicitly enable lead controller resource
jackjlli opened a new pull request #6725:
URL: https://github.com/apache/incubator-pinot/pull/6725
## Description
This PR explicitly enables lead controller resource.
The purpose of this feature is to separate hardware of Helix controller and Pinot controller as well as logically separate per-table functionality in all clusters.
After enabling lead controller resource, the controller host listed under `CONTROLLER/LEADER` ZNode only denotes the helix lead controller in the cluster.
The lead Pinot controller for each of the tables can be found by the following APIs:
Get the leaders for all the tables in the cluster
GET /leader/tables
Given a table name, return the partition id and lead controller instance id
GET /leader/tables/{tableName}
Details can be found in doc: https://cwiki.apache.org/confluence/display/PINOT/Controller+Separation+between+Helix+and+Pinot
## 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
- Deprecation of configurations
- Signature changes to public methods/interfaces
- 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
This PR explicitly enabled lead controller resource. The controller host listed in `CONTROLLER/LEADER` ZNode only denotes the helix lead controller in the cluster. The actual logic (like controller periodic tasks, realtime segment completion) of Pinot lead controller is distributed to each of the Pinot only controllers.
After enabling this feature, Helix controller and Pinot controller don't have to be run in the same hardware.
## Documentation
See https://cwiki.apache.org/confluence/display/PINOT/Controller+Separation+between+Helix+and+Pinot
--
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] jackjlli commented on a change in pull request #6725: Explicitly enable lead controller resource
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6725:
URL: https://github.com/apache/incubator-pinot/pull/6725#discussion_r603615724
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/util/HelixSetupUtils.java
##########
@@ -205,9 +205,10 @@ private static void createLeadControllerResourceIfNeeded(String helixClusterName
if (resourceConfig == null) {
resourceConfig = new ResourceConfig(LEAD_CONTROLLER_RESOURCE_NAME);
}
- // Set RESOURCE_ENABLED to false if it's absent in resource config
- if (resourceConfig.getSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY) == null) {
- resourceConfig.putSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY, Boolean.FALSE.toString());
+ // Explicitly set RESOURCE_ENABLED to true
+ String leadControllerResourceEnabled = resourceConfig.getSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY);
+ if (!Boolean.parseBoolean(leadControllerResourceEnabled)) {
Review comment:
Discussed offline. We want to enable this feature to keep the behavior consistent in all clusters. Thus, we enable the feature in this PR. In the next official release, we'll remove the config to always use this lead controller resource feature.
--
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] jackjlli commented on a change in pull request #6725: Explicitly enable lead controller resource
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6725:
URL: https://github.com/apache/incubator-pinot/pull/6725#discussion_r604293302
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/util/HelixSetupUtils.java
##########
@@ -205,10 +205,10 @@ private static void createLeadControllerResourceIfNeeded(String helixClusterName
if (resourceConfig == null) {
resourceConfig = new ResourceConfig(LEAD_CONTROLLER_RESOURCE_NAME);
}
- // Set RESOURCE_ENABLED to false if it's absent in resource config
- if (resourceConfig.getSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY) == null) {
- resourceConfig.putSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY, Boolean.FALSE.toString());
- }
+ // Explicitly set RESOURCE_ENABLED to true, so that the lead controller resource is always enabled.
+ // This is to help keep the behavior consistent in all clusters for better maintenance.
+ // TODO: remove the logic of handling this config in both controller and server in the next official release.
Review comment:
Created the issue https://github.com/apache/incubator-pinot/issues/6730 and linked it to this PR.
--
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 #6725: Explicitly enable lead controller resource
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6725:
URL: https://github.com/apache/incubator-pinot/pull/6725#discussion_r603671687
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/util/HelixSetupUtils.java
##########
@@ -205,10 +205,10 @@ private static void createLeadControllerResourceIfNeeded(String helixClusterName
if (resourceConfig == null) {
resourceConfig = new ResourceConfig(LEAD_CONTROLLER_RESOURCE_NAME);
}
- // Set RESOURCE_ENABLED to false if it's absent in resource config
- if (resourceConfig.getSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY) == null) {
- resourceConfig.putSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY, Boolean.FALSE.toString());
- }
+ // Explicitly set RESOURCE_ENABLED to true, so that the lead controller resource is always enabled.
+ // This is to help keep the behavior consistent in all clusters for better maintenance.
+ // TODO: remove the logic of handling this config in both controller and server in the next official release.
Review comment:
Can you please clarify "in the next release"? Do you want it removed after the next release?
--
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] jackjlli commented on a change in pull request #6725: Explicitly enable lead controller resource
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6725:
URL: https://github.com/apache/incubator-pinot/pull/6725#discussion_r603649554
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/util/HelixSetupUtils.java
##########
@@ -205,10 +205,10 @@ private static void createLeadControllerResourceIfNeeded(String helixClusterName
if (resourceConfig == null) {
resourceConfig = new ResourceConfig(LEAD_CONTROLLER_RESOURCE_NAME);
}
- // Set RESOURCE_ENABLED to false if it's absent in resource config
- if (resourceConfig.getSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY) == null) {
- resourceConfig.putSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY, Boolean.FALSE.toString());
- }
+ // Explicitly set RESOURCE_ENABLED to true, so that the lead controller resource is always enabled.
+ // This is to help keep the behavior consistent in all clusters for better maintenance.
+ // TODO: remove the logic of handling this config in both controller and server in the next official release.
Review comment:
Updated the description of this PR.
--
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 #6725: Explicitly enable lead controller resource
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6725:
URL: https://github.com/apache/incubator-pinot/pull/6725#discussion_r603671962
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/util/HelixSetupUtils.java
##########
@@ -205,10 +205,10 @@ private static void createLeadControllerResourceIfNeeded(String helixClusterName
if (resourceConfig == null) {
resourceConfig = new ResourceConfig(LEAD_CONTROLLER_RESOURCE_NAME);
}
- // Set RESOURCE_ENABLED to false if it's absent in resource config
- if (resourceConfig.getSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY) == null) {
- resourceConfig.putSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY, Boolean.FALSE.toString());
- }
+ // Explicitly set RESOURCE_ENABLED to true, so that the lead controller resource is always enabled.
+ // This is to help keep the behavior consistent in all clusters for better maintenance.
+ // TODO: remove the logic of handling this config in both controller and server in the next official release.
Review comment:
It is best if you identify all the code that needs to be removed, and put a comment head saying "Remove this block of code once 0.8.0 release is done" or something like that.
--
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 #6725: Explicitly enable lead controller resource
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6725:
URL: https://github.com/apache/incubator-pinot/pull/6725#discussion_r603645241
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/util/HelixSetupUtils.java
##########
@@ -205,10 +205,10 @@ private static void createLeadControllerResourceIfNeeded(String helixClusterName
if (resourceConfig == null) {
resourceConfig = new ResourceConfig(LEAD_CONTROLLER_RESOURCE_NAME);
}
- // Set RESOURCE_ENABLED to false if it's absent in resource config
- if (resourceConfig.getSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY) == null) {
- resourceConfig.putSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY, Boolean.FALSE.toString());
- }
+ // Explicitly set RESOURCE_ENABLED to true, so that the lead controller resource is always enabled.
+ // This is to help keep the behavior consistent in all clusters for better maintenance.
+ // TODO: remove the logic of handling this config in both controller and server in the next official release.
Review comment:
I think you have most of these already, just clarify the comments with the exact release number (e.g. this code is to be removed whenever we release 0.8 or later version of Pinot)
--
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] jackjlli merged pull request #6725: Explicitly enable lead controller resource
Posted by GitBox <gi...@apache.org>.
jackjlli merged pull request #6725:
URL: https://github.com/apache/incubator-pinot/pull/6725
--
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 #6725: Explicitly enable lead controller resource
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6725:
URL: https://github.com/apache/incubator-pinot/pull/6725#discussion_r603644640
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/util/HelixSetupUtils.java
##########
@@ -205,10 +205,10 @@ private static void createLeadControllerResourceIfNeeded(String helixClusterName
if (resourceConfig == null) {
resourceConfig = new ResourceConfig(LEAD_CONTROLLER_RESOURCE_NAME);
}
- // Set RESOURCE_ENABLED to false if it's absent in resource config
- if (resourceConfig.getSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY) == null) {
- resourceConfig.putSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY, Boolean.FALSE.toString());
- }
+ // Explicitly set RESOURCE_ENABLED to true, so that the lead controller resource is always enabled.
+ // This is to help keep the behavior consistent in all clusters for better maintenance.
+ // TODO: remove the logic of handling this config in both controller and server in the next official release.
Review comment:
Clarify "in the next official release". Do you want to remove it _after_ the next release is done, or _before_ the next release? If _before_, then you can do it now.
Also, please tag this PR for release notes and add appropriate comments that can be noted by whoever is building the release. Questions to be addressed in checkin comments:
- What is the difference in new cluster creation before and after this commit?
- How will existing clusters behave after the release when this code is installed?
- How to find out which controller is "responsible" for a table? (add link to the doc to the rest api. Doc it if it is not already there)
Add a pointer to the design doc.
--
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 #6725: Explicitly enable lead controller resource
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6725:
URL: https://github.com/apache/incubator-pinot/pull/6725#discussion_r603577211
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/util/HelixSetupUtils.java
##########
@@ -205,9 +205,10 @@ private static void createLeadControllerResourceIfNeeded(String helixClusterName
if (resourceConfig == null) {
resourceConfig = new ResourceConfig(LEAD_CONTROLLER_RESOURCE_NAME);
}
- // Set RESOURCE_ENABLED to false if it's absent in resource config
- if (resourceConfig.getSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY) == null) {
- resourceConfig.putSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY, Boolean.FALSE.toString());
+ // Explicitly set RESOURCE_ENABLED to true
+ String leadControllerResourceEnabled = resourceConfig.getSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY);
+ if (!Boolean.parseBoolean(leadControllerResourceEnabled)) {
Review comment:
We should only set this flag to true if it is not explicitly configured. The current logic will ignore the current config and always override the config
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io commented on pull request #6725: Explicitly enable lead controller resource
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6725:
URL: https://github.com/apache/incubator-pinot/pull/6725#issuecomment-809753264
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6725?src=pr&el=h1) Report
> Merging [#6725](https://codecov.io/gh/apache/incubator-pinot/pull/6725?src=pr&el=desc) (8603bff) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/8dbb70ba08daf90f5e9067fcec545203ffefe215?el=desc) (8dbb70b) will **decrease** coverage by `30.21%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6725?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6725 +/- ##
===========================================
- Coverage 73.83% 43.61% -30.22%
===========================================
Files 1396 1402 +6
Lines 67765 67915 +150
Branches 9807 9821 +14
===========================================
- Hits 50035 29622 -20413
- Misses 14485 35816 +21331
+ Partials 3245 2477 -768
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `43.61% <100.00%> (+0.09%)` | :arrow_up: |
| unittests | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6725?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/controller/helix/core/util/HelixSetupUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6725/diff?src=pr&el=tree#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3V0aWwvSGVsaXhTZXR1cFV0aWxzLmphdmE=) | `86.51% <100.00%> (-1.27%)` | :arrow_down: |
| [...c/main/java/org/apache/pinot/common/tier/Tier.java](https://codecov.io/gh/apache/incubator-pinot/pull/6725/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/6725/diff?src=pr&el=tree#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/stream/LongMsgOffset.java](https://codecov.io/gh/apache/incubator-pinot/pull/6725/diff?src=pr&el=tree#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvc3RyZWFtL0xvbmdNc2dPZmZzZXQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/core/util/fst/FSTBuilder.java](https://codecov.io/gh/apache/incubator-pinot/pull/6725/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL2ZzdC9GU1RCdWlsZGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/spi/config/table/TableType.java](https://codecov.io/gh/apache/incubator-pinot/pull/6725/diff?src=pr&el=tree#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL1RhYmxlVHlwZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/spi/data/DateTimeFieldSpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/6725/diff?src=pr&el=tree#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9EYXRlVGltZUZpZWxkU3BlYy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/spi/env/PinotConfiguration.java](https://codecov.io/gh/apache/incubator-pinot/pull/6725/diff?src=pr&el=tree#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZW52L1Bpbm90Q29uZmlndXJhdGlvbi5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/core/upsert/RecordLocation.java](https://codecov.io/gh/apache/incubator-pinot/pull/6725/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91cHNlcnQvUmVjb3JkTG9jYXRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../org/apache/pinot/spi/data/DimensionFieldSpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/6725/diff?src=pr&el=tree#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9EaW1lbnNpb25GaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [896 more](https://codecov.io/gh/apache/incubator-pinot/pull/6725/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6725?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6725?src=pr&el=footer). Last update [8dbb70b...8603bff](https://codecov.io/gh/apache/incubator-pinot/pull/6725?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #6725: Explicitly enable lead controller resource
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6725:
URL: https://github.com/apache/incubator-pinot/pull/6725#discussion_r603615724
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/util/HelixSetupUtils.java
##########
@@ -205,9 +205,10 @@ private static void createLeadControllerResourceIfNeeded(String helixClusterName
if (resourceConfig == null) {
resourceConfig = new ResourceConfig(LEAD_CONTROLLER_RESOURCE_NAME);
}
- // Set RESOURCE_ENABLED to false if it's absent in resource config
- if (resourceConfig.getSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY) == null) {
- resourceConfig.putSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY, Boolean.FALSE.toString());
+ // Explicitly set RESOURCE_ENABLED to true
+ String leadControllerResourceEnabled = resourceConfig.getSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY);
+ if (!Boolean.parseBoolean(leadControllerResourceEnabled)) {
Review comment:
Discussed offline. We want to enable this feature in all the clusters to keep the behavior consistent in all cluster. Thus, we enable the feature in this PR. In the next official release, we'll remove the config to always use this lead controller resource feature.
--
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] jackjlli commented on a change in pull request #6725: Explicitly enable lead controller resource
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6725:
URL: https://github.com/apache/incubator-pinot/pull/6725#discussion_r603580279
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/util/HelixSetupUtils.java
##########
@@ -205,9 +205,10 @@ private static void createLeadControllerResourceIfNeeded(String helixClusterName
if (resourceConfig == null) {
resourceConfig = new ResourceConfig(LEAD_CONTROLLER_RESOURCE_NAME);
}
- // Set RESOURCE_ENABLED to false if it's absent in resource config
- if (resourceConfig.getSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY) == null) {
- resourceConfig.putSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY, Boolean.FALSE.toString());
+ // Explicitly set RESOURCE_ENABLED to true
+ String leadControllerResourceEnabled = resourceConfig.getSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY);
+ if (!Boolean.parseBoolean(leadControllerResourceEnabled)) {
Review comment:
Yes, we should always override the config to true. The `if` statement above checks whether the config is empty or false; if that's the case, we set it to true. If the config is already true, do nothing.
The null check has been handled in `Boolean.parseBoolean`, thus merging the logic to one place.
--
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] jackjlli commented on a change in pull request #6725: Explicitly enable lead controller resource
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6725:
URL: https://github.com/apache/incubator-pinot/pull/6725#discussion_r603615724
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/util/HelixSetupUtils.java
##########
@@ -205,9 +205,10 @@ private static void createLeadControllerResourceIfNeeded(String helixClusterName
if (resourceConfig == null) {
resourceConfig = new ResourceConfig(LEAD_CONTROLLER_RESOURCE_NAME);
}
- // Set RESOURCE_ENABLED to false if it's absent in resource config
- if (resourceConfig.getSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY) == null) {
- resourceConfig.putSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY, Boolean.FALSE.toString());
+ // Explicitly set RESOURCE_ENABLED to true
+ String leadControllerResourceEnabled = resourceConfig.getSimpleConfig(LEAD_CONTROLLER_RESOURCE_ENABLED_KEY);
+ if (!Boolean.parseBoolean(leadControllerResourceEnabled)) {
Review comment:
Discussed offline. We want to enable this feature to keep the behavior consistent in all clusters. Thus, we enable the feature in this PR. In the next official release (0.8.0 or higher), we'll remove the config to always use this lead controller resource feature.
--
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