You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/06/16 01:29:03 UTC
[GitHub] [incubator-pinot] dongxiaoman opened a new pull request #7064: Allow updating controller and broker helix hostname
dongxiaoman opened a new pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064
### Summary
**Allow us to update Helix `hostname` property so controllers and brokers can be replaced easily.**
Right now Pinotserver has the feature to update hostname in Helix, so we can re-use the instance-id with a different FQDN hostname, but Pinot broker/controller are still bound to the Helix limitation of naming convention.
In this PR we introduce a few configuration properties so we can update Controller and Broker's hostnames.
This PR was tested on our AWS fleet successfully.
### Background
Pinot/Helix has a limitation that the Helix `"instanceId"` has to follow some naming format like "Controller_hostname4_9000" where the first part is host "type", second part is host name, and 3rd part is port.
This caused trouble for us in the early days when EC2 hosts are replaced with new hosts and hostnames change.
With my company's cloud infra in EC2, we replace hosts frequently, meaning hosts are taken down and new hosts join cluster very often. Every time a new host is swapped in, it shows up in Pinot as a brand new "participant" in cluster, and the old hosts are "dead" forever. This caused many operation pain.
To work around this issue, when a new host is replaced, we keep track of the old "instanceId", but register a new hostname into Helix cluster with the same Id.
### Technical details
The limitation is caused by Helix `0.9.8`'s assumption of inferring hostname from instanceId with a fixed format, around https://github.com/apache/helix/blob/helix-0.9.300-release/helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java#L146
### Changes For Pinot cluster
<img width="752" alt="Screen Shot 2021-06-15 at 4 18 33 PM" src="https://user-images.githubusercontent.com/11821736/122143928-4e60ed00-ce07-11eb-86d5-06c39d1c6161.png">
### Motivation
<!-- Why are you making this change? This can be a link to a Jira task. -->
Reduce one dependency, and also makes Pinot better.
Pinot controller helix hostname cannot be customized
### Testing
Tried out in our own fleet.
## 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
<!-- 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. -->
Adds
<!-- 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] Jackie-Jiang merged pull request #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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 #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r663169144
##########
File path: pinot-minion/src/main/java/org/apache/pinot/minion/BaseMinionStarter.java
##########
@@ -240,6 +240,16 @@ public String getStatusDescription() {
LOGGER.info("Pinot minion started");
}
+ private void updateInstanceConfigIfNeeded() {
+ InstanceConfig instanceConfig = HelixHelper.getInstanceConfig(_helixManager, _instanceId);
+ boolean updated = HelixHelper.updateHostnamePort(instanceConfig, _host, _port);
+ updated |= HelixHelper.addDefaultTags(instanceConfig,
Review comment:
Short circuit won't apply to `|` but only to `||`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] dongxiaoman commented on a change in pull request #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r662784424
##########
File path: pinot-minion/src/main/java/org/apache/pinot/minion/BaseMinionStarter.java
##########
@@ -240,6 +240,16 @@ public String getStatusDescription() {
LOGGER.info("Pinot minion started");
}
+ private void updateInstanceConfigIfNeeded() {
+ InstanceConfig instanceConfig = HelixHelper.getInstanceConfig(_helixManager, _instanceId);
+ boolean updated = HelixHelper.updateHostnamePort(instanceConfig, _host, _port);
+ updated |= HelixHelper.addDefaultTags(instanceConfig,
Review comment:
I thought about this but will the "short circuit" feature stop the function from calling in some cases? @Jackie-Jiang
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] dongxiaoman commented on a change in pull request #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r659189419
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java
##########
@@ -396,6 +396,7 @@
// but we keep this default for backward compatibility in case someone relies on this format
// see Server or Broker class for correct prefix format you should use
public static final String DEFAULT_METRICS_PREFIX = "pinot.controller.";
+ public static final String CONTROLLER_HELIX_INSTANCE_ID = "pinot.controller.helix.instance.id";
Review comment:
Changed.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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 #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#issuecomment-861995023
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 [#7064](https://codecov.io/gh/apache/incubator-pinot/pull/7064?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6f583f0) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/ac8f098e8ff883abb8323391480039f3a5b8aab7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ac8f098) will **decrease** coverage by `8.04%`.
> The diff coverage is `59.79%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 #7064 +/- ##
============================================
- Coverage 73.49% 65.45% -8.05%
Complexity 91 91
============================================
Files 1492 1495 +3
Lines 73422 73414 -8
Branches 10574 10581 +7
============================================
- Hits 53960 48050 -5910
- Misses 15927 21971 +6044
+ Partials 3535 3393 -142
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `?` | |
| unittests | `65.45% <59.79%> (-0.10%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/7064?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ava/org/apache/pinot/minion/BaseMinionStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vQmFzZU1pbmlvblN0YXJ0ZXIuamF2YQ==) | `0.00% <0.00%> (-78.19%)` | :arrow_down: |
| [.../pinot/server/starter/helix/BaseServerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9CYXNlU2VydmVyU3RhcnRlci5qYXZh) | `0.00% <0.00%> (-54.82%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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=) | `30.35% <ø> (ø)` | |
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `49.55% <33.33%> (-3.63%)` | :arrow_down: |
| [...e/pinot/broker/broker/helix/BaseBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jhc2VCcm9rZXJTdGFydGVyLmphdmE=) | `71.02% <76.19%> (-1.24%)` | :arrow_down: |
| [...apache/pinot/controller/BaseControllerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9CYXNlQ29udHJvbGxlclN0YXJ0ZXIuamF2YQ==) | `76.49% <81.81%> (-5.36%)` | :arrow_down: |
| [...g/apache/pinot/common/utils/helix/HelixHelper.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaGVsaXgvSGVsaXhIZWxwZXIuamF2YQ==) | `58.82% <96.96%> (+3.00%)` | :arrow_up: |
| [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [424 more](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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/7064?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/7064?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 [ac8f098...6f583f0](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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 #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#issuecomment-861995023
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 [#7064](https://codecov.io/gh/apache/incubator-pinot/pull/7064?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (318b685) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/ac8f098e8ff883abb8323391480039f3a5b8aab7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ac8f098) will **decrease** coverage by `31.77%`.
> The diff coverage is `81.42%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 #7064 +/- ##
=============================================
- Coverage 73.49% 41.72% -31.78%
+ Complexity 91 7 -84
=============================================
Files 1492 1492
Lines 73422 73445 +23
Branches 10574 10574
=============================================
- Hits 53960 30643 -23317
- Misses 15927 40236 +24309
+ Partials 3535 2566 -969
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `41.72% <81.42%> (+0.20%)` | :arrow_up: |
| unittests | `?` | |
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/7064?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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=) | `0.00% <ø> (-30.36%)` | :arrow_down: |
| [...apache/pinot/controller/BaseControllerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9CYXNlQ29udHJvbGxlclN0YXJ0ZXIuamF2YQ==) | `77.44% <66.66%> (-4.41%)` | :arrow_down: |
| [...g/apache/pinot/common/utils/helix/HelixHelper.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaGVsaXgvSGVsaXhIZWxwZXIuamF2YQ==) | `45.45% <75.67%> (-10.36%)` | :arrow_down: |
| [.../pinot/server/starter/helix/BaseServerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9CYXNlU2VydmVyU3RhcnRlci5qYXZh) | `54.43% <90.00%> (-0.38%)` | :arrow_down: |
| [...e/pinot/broker/broker/helix/BaseBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jhc2VCcm9rZXJTdGFydGVyLmphdmE=) | `72.25% <90.90%> (ø)` | |
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `45.70% <100.00%> (-7.49%)` | :arrow_down: |
| [...ava/org/apache/pinot/minion/BaseMinionStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vQmFzZU1pbmlvblN0YXJ0ZXIuamF2YQ==) | `79.24% <100.00%> (+1.06%)` | :arrow_up: |
| [...c/main/java/org/apache/pinot/common/tier/Tier.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-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/7064/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/BigDecimalUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQmlnRGVjaW1hbFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [952 more](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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/7064?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/7064?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 [ac8f098...318b685](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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 #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#issuecomment-861995023
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 [#7064](https://codecov.io/gh/apache/incubator-pinot/pull/7064?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cef491b) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/18051eb9fd7f28e2a4c84ae689345205023e9cd0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (18051eb) will **decrease** coverage by `8.04%`.
> The diff coverage is `61.76%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 #7064 +/- ##
============================================
- Coverage 73.67% 65.63% -8.05%
Complexity 91 91
============================================
Files 1479 1480 +1
Lines 72849 72883 +34
Branches 10481 10482 +1
============================================
- Hits 53674 47836 -5838
- Misses 15709 21690 +5981
+ Partials 3466 3357 -109
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `?` | |
| unittests | `65.63% <61.76%> (+0.07%)` | :arrow_up: |
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/7064?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...in/java/org/apache/pinot/minion/MinionStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vTWluaW9uU3RhcnRlci5qYXZh) | `0.00% <0.00%> (-74.79%)` | :arrow_down: |
| [...pinot/server/starter/helix/HelixServerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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%> (-53.62%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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=) | `29.82% <ø> (ø)` | |
| [...org/apache/pinot/controller/ControllerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyU3RhcnRlci5qYXZh) | `69.13% <60.00%> (-5.24%)` | :arrow_down: |
| [...g/apache/pinot/common/utils/helix/HelixHelper.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaGVsaXgvSGVsaXhIZWxwZXIuamF2YQ==) | `56.25% <77.77%> (+0.43%)` | :arrow_up: |
| [.../pinot/broker/broker/helix/HelixBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0hlbGl4QnJva2VyU3RhcnRlci5qYXZh) | `69.10% <83.33%> (+0.52%)` | :arrow_up: |
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `50.68% <100.00%> (-2.99%)` | :arrow_down: |
| [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [359 more](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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/7064?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/7064?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 [18051eb...cef491b](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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] dongxiaoman commented on a change in pull request #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r654065894
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -330,6 +334,27 @@ public void start()
LOGGER.info("Finish starting Pinot broker");
}
+ @VisibleForTesting
+ static void updateHelixHost(PinotConfiguration brokerConf, HelixManager helixManager, String clusterName, String brokerInstanceId) {
+ boolean allowUpdateHelixHost = brokerConf.getProperty(Broker.BROKER_DYNAMIC_HELIX_HOST, false);
Review comment:
Removed.
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java
##########
@@ -199,6 +199,9 @@
public static final String BROKER_TLS_PREFIX = "pinot.broker.tls";
public static final String BROKER_NETTYTLS_ENABLED = "pinot.broker.nettytls.enabled";
+ public static final String BROKER_DYNAMIC_HELIX_HOST = "pinot.broker.dynamic.helix.host";
+ public static final String BROKER_NETTY_PORT = "pinot.broker.netty.port";
+ public static final String BROKER_NETTY_HOST = "pinot.broker.netty.host";
Review comment:
let me double check that; forgot this comment
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java
##########
@@ -199,6 +199,9 @@
public static final String BROKER_TLS_PREFIX = "pinot.broker.tls";
public static final String BROKER_NETTYTLS_ENABLED = "pinot.broker.nettytls.enabled";
+ public static final String BROKER_DYNAMIC_HELIX_HOST = "pinot.broker.dynamic.helix.host";
+ public static final String BROKER_NETTY_PORT = "pinot.broker.netty.port";
+ public static final String BROKER_NETTY_HOST = "pinot.broker.netty.host";
Review comment:
Removed now
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java
##########
@@ -396,6 +399,8 @@
// but we keep this default for backward compatibility in case someone relies on this format
// see Server or Broker class for correct prefix format you should use
public static final String DEFAULT_METRICS_PREFIX = "pinot.controller.";
+ public static final String CONTROLLER_DYNAMIC_HELIX_HOST = "pinot.controller.dynamic.helix.host";
Review comment:
Removed
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -319,6 +322,8 @@ public void start()
new BrokerUserDefinedMessageHandlerFactory(_routingManager, queryQuotaManager));
_participantHelixManager.connect();
addInstanceTagIfNeeded();
+ updateHelixHost(_brokerConf, _participantHelixManager, _clusterName, _brokerId);
Review comment:
Yes, a good point; I refactored the code so now the 3 host types are using the same function now.
##########
File path: pinot-minion/src/main/java/org/apache/pinot/minion/MinionStarter.java
##########
@@ -225,7 +226,13 @@ public void start()
_helixManager.getStateMachineEngine().registerStateModelFactory("Task", new TaskStateModelFactory(_helixManager,
new TaskFactoryRegistry(_taskExecutorFactoryRegistry, _eventObserverFactoryRegistry).getTaskFactoryRegistry()));
_helixManager.connect();
- addInstanceTagIfNeeded();
+ HelixHelper.updateInstanceConfigIfNeeded(
Review comment:
please double check; I don't have minions setup so cannot verify it.
--
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] dongxiaoman commented on a change in pull request #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r659915596
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -53,9 +54,7 @@ public HelixBrokerStarter(PinotConfiguration brokerConf, String clusterName, Str
private static PinotConfiguration applyBrokerConfigs(PinotConfiguration brokerConf, String clusterName, String zkServers, @Nullable String brokerHost) {
brokerConf.setProperty(Helix.CONFIG_OF_CLUSTER_NAME, clusterName);
brokerConf.setProperty(Helix.CONFIG_OF_ZOOKEEPR_SERVER, zkServers);
- if (brokerHost == null) {
- brokerConf.clearProperty(Broker.CONFIG_OF_BROKER_HOSTNAME);
- } else {
+ if (!Strings.isNullOrEmpty(brokerHost)) {
Review comment:
Based on code reading, I don't see a use case that we need to clear the values. cc @Jackie-Jiang for double checking
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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 #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r654596456
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -139,12 +139,13 @@ public HelixBrokerStarter(PinotConfiguration brokerConf) throws Exception {
if (brokerHost == null) {
brokerHost = _brokerConf.getProperty(Helix.SET_INSTANCE_ID_TO_HOSTNAME_KEY, false) ? NetUtils
.getHostnameOrAddress() : NetUtils.getHostAddress();
+ brokerConf.setProperty(Broker.CONFIG_OF_BROKER_HOSTNAME, brokerHost);
Review comment:
Suggest making `brokerHost` a member variable instead of backfilling it into the config. Ideally the config should not be modified
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -266,65 +264,21 @@ private void registerServiceStatusHandler() {
}
private void updateInstanceConfigIfNeeded(String host, int port) {
- InstanceConfig instanceConfig = _helixAdmin.getInstanceConfig(_helixClusterName, _instanceId);
- boolean needToUpdateInstanceConfig = false;
-
- // Add default instance tags if not exist
- List<String> instanceTags = instanceConfig.getTags();
- if (instanceTags == null || instanceTags.size() == 0) {
- if (ZKMetadataProvider.getClusterTenantIsolationEnabled(_helixManager.getHelixPropertyStore())) {
- instanceConfig.addTag(TagNameUtils.getOfflineTagForTenant(null));
- instanceConfig.addTag(TagNameUtils.getRealtimeTagForTenant(null));
- } else {
- instanceConfig.addTag(Helix.UNTAGGED_SERVER_INSTANCE);
- }
- needToUpdateInstanceConfig = true;
- }
-
- // Update host and port if needed
- if (!host.equals(instanceConfig.getHostName())) {
- instanceConfig.setHostName(host);
- needToUpdateInstanceConfig = true;
- }
- String portStr = Integer.toString(port);
- if (!portStr.equals(instanceConfig.getPort())) {
- instanceConfig.setPort(portStr);
- needToUpdateInstanceConfig = true;
- }
-
- // Update instance config with environment properties
- if (_pinotEnvironmentProvider != null) {
Review comment:
(Critical) the environment properties update is missing
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java
##########
@@ -396,6 +396,7 @@
// but we keep this default for backward compatibility in case someone relies on this format
// see Server or Broker class for correct prefix format you should use
public static final String DEFAULT_METRICS_PREFIX = "pinot.controller.";
+ public static final String CONTROLLER_HELIX_INSTANCE_ID = "pinot.controller.helix.instance.id";
Review comment:
Seems we already have different naming convention for different instance, but let's not add another one.. Suggest using `pinot.controller.instance.id` instead
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -139,12 +139,13 @@ public HelixBrokerStarter(PinotConfiguration brokerConf) throws Exception {
if (brokerHost == null) {
brokerHost = _brokerConf.getProperty(Helix.SET_INSTANCE_ID_TO_HOSTNAME_KEY, false) ? NetUtils
.getHostnameOrAddress() : NetUtils.getHostAddress();
+ brokerConf.setProperty(Broker.CONFIG_OF_BROKER_HOSTNAME, brokerHost);
}
_brokerId = _brokerConf.getProperty(Helix.Instance.INSTANCE_ID_KEY,
- Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + _listenerConfigs.get(0).getPort());
+ Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + _listenerConfigs.get(0).getPort());
Review comment:
Suggest also putting `port` as a member variable
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +498,76 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Update host config in Helix if needed.
+ * The hostname and port cannot be null or empty;
+ * The port will be validated against integer.
+ * There is a callback lambda that can provide the tags if needed.
+ * For example () -> ImmutableList.of("Default_tenant")
+ * @param helixManager The Participant Manager
+ * @param instanceId the Helix instance id
+ * @param hostName the Hostname to update
+ * @param hostPort the host port to update
+ * @param getDefaultTags Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void updateInstanceConfigIfNeeded(HelixManager helixManager, String instanceId, String hostName, String hostPort, Supplier<List<String>> getDefaultTags) {
Review comment:
We can split this into multiple primitive operations and the individual starter can choose the primitives to use, e.g. `addDefaultInstanceTags()`, `updateHostnamePort()` etc
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -318,7 +319,22 @@ public void start()
.registerMessageHandlerFactory(Message.MessageType.USER_DEFINE_MSG.toString(),
new BrokerUserDefinedMessageHandlerFactory(_routingManager, queryQuotaManager));
_participantHelixManager.connect();
- addInstanceTagIfNeeded();
+
+ HelixHelper.updateInstanceConfigIfNeeded(
Review comment:
Please set-up the code format per: https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup
--
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 #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
xiangfu0 commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r659285548
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +498,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (defaultTags != null && !defaultTags.isEmpty()) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceId, instanceTags);
Review comment:
the order is switched, should be: instanceTags, instanceId
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] dongxiaoman commented on a change in pull request #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r660197626
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
Review comment:
Changed to `.isEmpty()`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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 #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#issuecomment-861995023
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 [#7064](https://codecov.io/gh/apache/incubator-pinot/pull/7064?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (786ccf6) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/ac8f098e8ff883abb8323391480039f3a5b8aab7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ac8f098) will **decrease** coverage by `8.05%`.
> The diff coverage is `60.78%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 #7064 +/- ##
============================================
- Coverage 73.49% 65.43% -8.06%
- Complexity 91 92 +1
============================================
Files 1492 1495 +3
Lines 73422 73437 +15
Branches 10574 10584 +10
============================================
- Hits 53960 48056 -5904
- Misses 15927 21987 +6060
+ Partials 3535 3394 -141
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `?` | |
| unittests | `65.43% <60.78%> (-0.11%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/7064?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ava/org/apache/pinot/minion/BaseMinionStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vQmFzZU1pbmlvblN0YXJ0ZXIuamF2YQ==) | `0.00% <0.00%> (-78.19%)` | :arrow_down: |
| [.../pinot/server/starter/helix/BaseServerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9CYXNlU2VydmVyU3RhcnRlci5qYXZh) | `0.00% <0.00%> (-54.82%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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=) | `30.35% <ø> (ø)` | |
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `49.55% <33.33%> (-3.63%)` | :arrow_down: |
| [...e/pinot/broker/broker/helix/BaseBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jhc2VCcm9rZXJTdGFydGVyLmphdmE=) | `71.02% <76.19%> (-1.24%)` | :arrow_down: |
| [...apache/pinot/controller/BaseControllerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9CYXNlQ29udHJvbGxlclN0YXJ0ZXIuamF2YQ==) | `76.41% <86.66%> (-5.44%)` | :arrow_down: |
| [...g/apache/pinot/common/utils/helix/HelixHelper.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaGVsaXgvSGVsaXhIZWxwZXIuamF2YQ==) | `58.82% <96.96%> (+3.00%)` | :arrow_up: |
| [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [427 more](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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/7064?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/7064?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 [ac8f098...786ccf6](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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 #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#issuecomment-861995023
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 [#7064](https://codecov.io/gh/apache/incubator-pinot/pull/7064?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8ad5f16) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/18051eb9fd7f28e2a4c84ae689345205023e9cd0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (18051eb) will **decrease** coverage by `31.55%`.
> The diff coverage is `73.68%`.
> :exclamation: Current head 8ad5f16 differs from pull request most recent head cef491b. Consider uploading reports for the commit cef491b to get more accurate results
[](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 #7064 +/- ##
=============================================
- Coverage 73.67% 42.12% -31.56%
+ Complexity 91 7 -84
=============================================
Files 1479 1480 +1
Lines 72849 72883 +34
Branches 10481 10482 +1
=============================================
- Hits 53674 30703 -22971
- Misses 15709 39636 +23927
+ Partials 3466 2544 -922
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `42.12% <73.68%> (+0.08%)` | :arrow_up: |
| unittests | `?` | |
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/7064?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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=) | `0.00% <ø> (-29.83%)` | :arrow_down: |
| [...rator/query/AggregationGroupByOrderByOperator.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9xdWVyeS9BZ2dyZWdhdGlvbkdyb3VwQnlPcmRlckJ5T3BlcmF0b3IuamF2YQ==) | `71.92% <38.46%> (-21.55%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/util/QueryOptions.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL1F1ZXJ5T3B0aW9ucy5qYXZh) | `60.00% <45.45%> (-31.67%)` | :arrow_down: |
| [...org/apache/pinot/controller/ControllerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyU3RhcnRlci5qYXZh) | `70.37% <60.00%> (-4.01%)` | :arrow_down: |
| [...or/dociditerators/ExpressionScanDocIdIterator.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9kb2NpZGl0ZXJhdG9ycy9FeHByZXNzaW9uU2NhbkRvY0lkSXRlcmF0b3IuamF2YQ==) | `62.85% <70.00%> (-4.26%)` | :arrow_down: |
| [...he/pinot/core/operator/BitmapDocIdSetOperator.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9CaXRtYXBEb2NJZFNldE9wZXJhdG9yLmphdmE=) | `75.00% <75.00%> (ø)` | |
| [...g/apache/pinot/common/utils/helix/HelixHelper.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaGVsaXgvSGVsaXhIZWxwZXIuamF2YQ==) | `46.15% <80.55%> (-9.67%)` | :arrow_down: |
| [.../pinot/broker/broker/helix/HelixBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0hlbGl4QnJva2VyU3RhcnRlci5qYXZh) | `68.58% <91.66%> (ø)` | |
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `46.11% <100.00%> (-7.56%)` | :arrow_down: |
| [.../core/operator/query/SelectionOrderByOperator.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9xdWVyeS9TZWxlY3Rpb25PcmRlckJ5T3BlcmF0b3IuamF2YQ==) | `94.73% <100.00%> (+0.25%)` | :arrow_up: |
| ... and [950 more](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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/7064?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/7064?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 [18051eb...cef491b](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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] dongxiaoman commented on a change in pull request #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r660198790
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
Review comment:
All updated.
Now the tracking logic is also in HelixHelper so the code for 4 host types are shared.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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 #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
xiangfu0 commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r659397636
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +498,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (defaultTags != null && !defaultTags.isEmpty()) {
Review comment:
use `(!CollectionUtils.isEmpty(defaultTags))` ?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +498,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (defaultTags != null && !defaultTags.isEmpty()) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceId, instanceTags);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager, String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager, InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+ helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()), instanceConfig),
+ "Failed to update instance config");
+ }
+
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String instanceId, String hostName, int hostPort) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ if (Strings.isNullOrEmpty(hostName)) {
+ LOGGER.info("host is empty, skip updating helix host.");
+ return false;
+ }
+ boolean updated = false;
+ if (!String.valueOf(hostPort).equals(instanceConfig.getPort())) {
+ instanceConfig.setPort(String.valueOf(hostPort));
+ updated = true;
+ }
+ // Update host and port if needed
Review comment:
move this comment before line 567
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -53,9 +54,7 @@ public HelixBrokerStarter(PinotConfiguration brokerConf, String clusterName, Str
private static PinotConfiguration applyBrokerConfigs(PinotConfiguration brokerConf, String clusterName, String zkServers, @Nullable String brokerHost) {
brokerConf.setProperty(Helix.CONFIG_OF_CLUSTER_NAME, clusterName);
brokerConf.setProperty(Helix.CONFIG_OF_ZOOKEEPR_SERVER, zkServers);
- if (brokerHost == null) {
- brokerConf.clearProperty(Broker.CONFIG_OF_BROKER_HOSTNAME);
- } else {
+ if (!Strings.isNullOrEmpty(brokerHost)) {
Review comment:
shall we also keep the logic to delete broker host? Or we expect this host/port to always be set?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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 #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#issuecomment-861995023
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 [#7064](https://codecov.io/gh/apache/incubator-pinot/pull/7064?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0bcd93a) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/ac8f098e8ff883abb8323391480039f3a5b8aab7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ac8f098) will **increase** coverage by `0.05%`.
> The diff coverage is `85.91%`.
> :exclamation: Current head 0bcd93a differs from pull request most recent head b69a5fb. Consider uploading reports for the commit b69a5fb to get more accurate results
[](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 #7064 +/- ##
============================================
+ Coverage 73.49% 73.55% +0.05%
Complexity 91 91
============================================
Files 1492 1492
Lines 73422 73444 +22
Branches 10574 10574
============================================
+ Hits 53960 54020 +60
+ Misses 15927 15903 -24
+ Partials 3535 3521 -14
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `41.74% <80.28%> (+0.22%)` | :arrow_up: |
| unittests | `65.58% <64.78%> (+0.04%)` | :arrow_up: |
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/7064?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../pinot/broker/broker/helix/HelixBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0hlbGl4QnJva2VyU3RhcnRlci5qYXZh) | `30.76% <0.00%> (+23.36%)` | :arrow_up: |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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=) | `30.35% <ø> (ø)` | |
| [...apache/pinot/controller/BaseControllerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9CYXNlQ29udHJvbGxlclN0YXJ0ZXIuamF2YQ==) | `81.48% <66.66%> (-0.37%)` | :arrow_down: |
| [...g/apache/pinot/common/utils/helix/HelixHelper.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaGVsaXgvSGVsaXhIZWxwZXIuamF2YQ==) | `61.24% <86.48%> (+5.43%)` | :arrow_up: |
| [.../pinot/server/starter/helix/BaseServerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9CYXNlU2VydmVyU3RhcnRlci5qYXZh) | `54.43% <90.00%> (-0.38%)` | :arrow_down: |
| [...e/pinot/broker/broker/helix/BaseBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jhc2VCcm9rZXJTdGFydGVyLmphdmE=) | `72.83% <90.90%> (+0.57%)` | :arrow_up: |
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `53.39% <100.00%> (+0.21%)` | :arrow_up: |
| [...ava/org/apache/pinot/minion/BaseMinionStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vQmFzZU1pbmlvblN0YXJ0ZXIuamF2YQ==) | `79.24% <100.00%> (+1.06%)` | :arrow_up: |
| [...ller/helix/core/minion/TaskTypeMetricsUpdater.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9UYXNrVHlwZU1ldHJpY3NVcGRhdGVyLmphdmE=) | `80.00% <0.00%> (-20.00%)` | :arrow_down: |
| [...impl/dictionary/DoubleOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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%> (-6.03%)` | :arrow_down: |
| ... and [30 more](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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/7064?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/7064?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 [ac8f098...b69a5fb](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] dongxiaoman commented on a change in pull request #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r659221902
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +498,76 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Update host config in Helix if needed.
+ * The hostname and port cannot be null or empty;
+ * The port will be validated against integer.
+ * There is a callback lambda that can provide the tags if needed.
+ * For example () -> ImmutableList.of("Default_tenant")
+ * @param helixManager The Participant Manager
+ * @param instanceId the Helix instance id
+ * @param hostName the Hostname to update
+ * @param hostPort the host port to update
+ * @param getDefaultTags Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void updateInstanceConfigIfNeeded(HelixManager helixManager, String instanceId, String hostName, String hostPort, Supplier<List<String>> getDefaultTags) {
Review comment:
Refactored now
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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 #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#issuecomment-861995023
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 [#7064](https://codecov.io/gh/apache/incubator-pinot/pull/7064?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f8cc5d5) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/ac8f098e8ff883abb8323391480039f3a5b8aab7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ac8f098) will **decrease** coverage by `31.88%`.
> The diff coverage is `86.48%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 #7064 +/- ##
=============================================
- Coverage 73.49% 41.60% -31.89%
+ Complexity 91 7 -84
=============================================
Files 1492 1493 +1
Lines 73422 73564 +142
Branches 10574 10594 +20
=============================================
- Hits 53960 30609 -23351
- Misses 15927 40369 +24442
+ Partials 3535 2586 -949
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `41.60% <86.48%> (+0.09%)` | :arrow_up: |
| unittests | `?` | |
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/7064?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../pinot/broker/broker/helix/HelixBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0hlbGl4QnJva2VyU3RhcnRlci5qYXZh) | `7.69% <0.00%> (+0.28%)` | :arrow_up: |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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=) | `0.00% <ø> (-30.36%)` | :arrow_down: |
| [...apache/pinot/controller/BaseControllerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9CYXNlQ29udHJvbGxlclN0YXJ0ZXIuamF2YQ==) | `77.44% <66.66%> (-4.41%)` | :arrow_down: |
| [...g/apache/pinot/common/utils/helix/HelixHelper.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaGVsaXgvSGVsaXhIZWxwZXIuamF2YQ==) | `48.59% <88.09%> (-7.22%)` | :arrow_down: |
| [.../pinot/server/starter/helix/BaseServerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9CYXNlU2VydmVyU3RhcnRlci5qYXZh) | `54.29% <88.88%> (-0.52%)` | :arrow_down: |
| [...e/pinot/broker/broker/helix/BaseBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jhc2VCcm9rZXJTdGFydGVyLmphdmE=) | `72.09% <90.90%> (-0.17%)` | :arrow_down: |
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `44.88% <100.00%> (-8.30%)` | :arrow_down: |
| [...ava/org/apache/pinot/minion/BaseMinionStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vQmFzZU1pbmlvblN0YXJ0ZXIuamF2YQ==) | `78.64% <100.00%> (+0.45%)` | :arrow_up: |
| [...c/main/java/org/apache/pinot/common/tier/Tier.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-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/7064/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [955 more](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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/7064?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/7064?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 [ac8f098...f8cc5d5](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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 #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#issuecomment-861995023
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 [#7064](https://codecov.io/gh/apache/incubator-pinot/pull/7064?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b54f511) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/ac8f098e8ff883abb8323391480039f3a5b8aab7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ac8f098) will **decrease** coverage by `0.04%`.
> The diff coverage is `85.91%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 #7064 +/- ##
============================================
- Coverage 73.49% 73.45% -0.05%
Complexity 91 91
============================================
Files 1492 1492
Lines 73422 73444 +22
Branches 10574 10574
============================================
- Hits 53960 53945 -15
- Misses 15927 15978 +51
+ Partials 3535 3521 -14
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `41.54% <80.28%> (+0.03%)` | :arrow_up: |
| unittests | `65.57% <64.78%> (+0.03%)` | :arrow_up: |
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/7064?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../pinot/broker/broker/helix/HelixBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0hlbGl4QnJva2VyU3RhcnRlci5qYXZh) | `30.76% <0.00%> (+23.36%)` | :arrow_up: |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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=) | `30.35% <ø> (ø)` | |
| [...apache/pinot/controller/BaseControllerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9CYXNlQ29udHJvbGxlclN0YXJ0ZXIuamF2YQ==) | `81.48% <66.66%> (-0.37%)` | :arrow_down: |
| [...g/apache/pinot/common/utils/helix/HelixHelper.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaGVsaXgvSGVsaXhIZWxwZXIuamF2YQ==) | `62.67% <86.48%> (+6.86%)` | :arrow_up: |
| [.../pinot/server/starter/helix/BaseServerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9CYXNlU2VydmVyU3RhcnRlci5qYXZh) | `54.43% <90.00%> (-0.38%)` | :arrow_down: |
| [...e/pinot/broker/broker/helix/BaseBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jhc2VCcm9rZXJTdGFydGVyLmphdmE=) | `72.83% <90.90%> (+0.57%)` | :arrow_up: |
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `53.39% <100.00%> (+0.21%)` | :arrow_up: |
| [...ava/org/apache/pinot/minion/BaseMinionStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vQmFzZU1pbmlvblN0YXJ0ZXIuamF2YQ==) | `79.24% <100.00%> (+1.06%)` | :arrow_up: |
| [...data/manager/realtime/SegmentCommitterFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvU2VnbWVudENvbW1pdHRlckZhY3RvcnkuamF2YQ==) | `64.70% <0.00%> (-35.30%)` | :arrow_down: |
| [...ller/helix/core/minion/TaskTypeMetricsUpdater.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9UYXNrVHlwZU1ldHJpY3NVcGRhdGVyLmphdmE=) | `80.00% <0.00%> (-20.00%)` | :arrow_down: |
| ... and [26 more](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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/7064?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/7064?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 [ac8f098...b54f511](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] dongxiaoman commented on a change in pull request #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r660200155
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
Review comment:
Added `NonNull` annotation and call directly.
If no tags, we can call something like `ImmutableList::of` directly
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] dongxiaoman commented on a change in pull request #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r660199802
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
Review comment:
Changed too.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] dongxiaoman commented on a change in pull request #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r660199512
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager, String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager, InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+ helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()), instanceConfig),
+ "Failed to update instance config");
+ }
+
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String instanceId, String hostName, int hostPort) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ if (Strings.isNullOrEmpty(hostName)) {
Review comment:
Changed to use `org.apache.commons.lang3.StringUtils`; and `hostName` with `port` are both tested
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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 #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#issuecomment-861995023
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 [#7064](https://codecov.io/gh/apache/incubator-pinot/pull/7064?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (786ccf6) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/ac8f098e8ff883abb8323391480039f3a5b8aab7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ac8f098) will **increase** coverage by `0.07%`.
> The diff coverage is `85.29%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 #7064 +/- ##
============================================
+ Coverage 73.49% 73.56% +0.07%
- Complexity 91 92 +1
============================================
Files 1492 1495 +3
Lines 73422 73437 +15
Branches 10574 10584 +10
============================================
+ Hits 53960 54024 +64
+ Misses 15927 15903 -24
+ Partials 3535 3510 -25
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `41.75% <83.33%> (+0.24%)` | :arrow_up: |
| unittests | `65.43% <60.78%> (-0.11%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/7064?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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=) | `30.35% <ø> (ø)` | |
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `52.65% <33.33%> (-0.53%)` | :arrow_down: |
| [.../pinot/server/starter/helix/BaseServerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9CYXNlU2VydmVyU3RhcnRlci5qYXZh) | `54.92% <65.00%> (+0.11%)` | :arrow_up: |
| [...apache/pinot/controller/BaseControllerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9CYXNlQ29udHJvbGxlclN0YXJ0ZXIuamF2YQ==) | `81.72% <86.66%> (-0.13%)` | :arrow_down: |
| [...ava/org/apache/pinot/minion/BaseMinionStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vQmFzZU1pbmlvblN0YXJ0ZXIuamF2YQ==) | `78.89% <90.00%> (+0.71%)` | :arrow_up: |
| [...e/pinot/broker/broker/helix/BaseBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jhc2VCcm9rZXJTdGFydGVyLmphdmE=) | `73.29% <90.47%> (+1.04%)` | :arrow_up: |
| [...g/apache/pinot/common/utils/helix/HelixHelper.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaGVsaXgvSGVsaXhIZWxwZXIuamF2YQ==) | `62.25% <96.96%> (+6.44%)` | :arrow_up: |
| [...ion/tasks/BaseSingleSegmentConversionExecutor.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvQmFzZVNpbmdsZVNlZ21lbnRDb252ZXJzaW9uRXhlY3V0b3IuamF2YQ==) | `84.61% <0.00%> (-8.87%)` | :arrow_down: |
| [...e/pinot/plugin/stream/kinesis/KinesisConsumer.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtcGx1Z2lucy9waW5vdC1zdHJlYW0taW5nZXN0aW9uL3Bpbm90LWtpbmVzaXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9zdHJlYW0va2luZXNpcy9LaW5lc2lzQ29uc3VtZXIuamF2YQ==) | `34.65% <0.00%> (-8.21%)` | :arrow_down: |
| [...he/pinot/plugin/minion/tasks/BaseTaskExecutor.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvQmFzZVRhc2tFeGVjdXRvci5qYXZh) | `76.47% <0.00%> (-6.87%)` | :arrow_down: |
| ... and [129 more](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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/7064?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/7064?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 [ac8f098...786ccf6](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] dongxiaoman commented on a change in pull request #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r660199258
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager, String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager, InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+ helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()), instanceConfig),
+ "Failed to update instance config");
Review comment:
Added.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager, String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager, InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+ helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()), instanceConfig),
+ "Failed to update instance config");
+ }
+
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String instanceId, String hostName, int hostPort) {
Review comment:
Changed
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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 #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#issuecomment-861995023
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 [#7064](https://codecov.io/gh/apache/incubator-pinot/pull/7064?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fd4012f) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/0a020b4558c815490b01516fb41556e6f9420406?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0a020b4) will **decrease** coverage by `31.54%`.
> The diff coverage is `21.21%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 #7064 +/- ##
=============================================
- Coverage 73.59% 42.04% -31.55%
+ Complexity 91 7 -84
=============================================
Files 1477 1477
Lines 72800 72832 +32
Branches 10471 10475 +4
=============================================
- Hits 53576 30623 -22953
- Misses 15761 39653 +23892
+ Partials 3463 2556 -907
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `42.04% <21.21%> (+0.18%)` | :arrow_up: |
| unittests | `?` | |
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/7064?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...g/apache/pinot/common/utils/helix/HelixHelper.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaGVsaXgvSGVsaXhIZWxwZXIuamF2YQ==) | `37.22% <0.00%> (-18.60%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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=) | `0.00% <ø> (-29.83%)` | :arrow_down: |
| [...org/apache/pinot/controller/ControllerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyU3RhcnRlci5qYXZh) | `68.88% <25.00%> (-5.50%)` | :arrow_down: |
| [.../pinot/broker/broker/helix/HelixBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0hlbGl4QnJva2VyU3RhcnRlci5qYXZh) | `65.84% <27.27%> (-2.75%)` | :arrow_down: |
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `45.90% <50.00%> (-7.77%)` | :arrow_down: |
| [...c/main/java/org/apache/pinot/common/tier/Tier.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-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/7064/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/BigDecimalUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQmlnRGVjaW1hbFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/common/tier/TierFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyRmFjdG9yeS5qYXZh) | `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/7064/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL1RhYmxlVHlwZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [936 more](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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/7064?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/7064?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 [0a020b4...fd4012f](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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] codecov-commenter edited a comment on pull request #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#issuecomment-861995023
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 [#7064](https://codecov.io/gh/apache/incubator-pinot/pull/7064?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b54f511) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/ac8f098e8ff883abb8323391480039f3a5b8aab7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ac8f098) will **decrease** coverage by `7.91%`.
> The diff coverage is `64.78%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 #7064 +/- ##
============================================
- Coverage 73.49% 65.57% -7.92%
Complexity 91 91
============================================
Files 1492 1492
Lines 73422 73444 +22
Branches 10574 10574
============================================
- Hits 53960 48160 -5800
- Misses 15927 21879 +5952
+ Partials 3535 3405 -130
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `?` | |
| unittests | `65.57% <64.78%> (+0.03%)` | :arrow_up: |
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/7064?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../pinot/broker/broker/helix/HelixBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0hlbGl4QnJva2VyU3RhcnRlci5qYXZh) | `30.76% <0.00%> (+23.36%)` | :arrow_up: |
| [...ava/org/apache/pinot/minion/BaseMinionStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vQmFzZU1pbmlvblN0YXJ0ZXIuamF2YQ==) | `0.00% <0.00%> (-78.19%)` | :arrow_down: |
| [.../pinot/server/starter/helix/BaseServerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9CYXNlU2VydmVyU3RhcnRlci5qYXZh) | `0.00% <0.00%> (-54.82%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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=) | `30.35% <ø> (ø)` | |
| [...apache/pinot/controller/BaseControllerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9CYXNlQ29udHJvbGxlclN0YXJ0ZXIuamF2YQ==) | `76.09% <66.66%> (-5.76%)` | :arrow_down: |
| [...e/pinot/broker/broker/helix/BaseBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jhc2VCcm9rZXJTdGFydGVyLmphdmE=) | `70.52% <81.81%> (-1.74%)` | :arrow_down: |
| [...g/apache/pinot/common/utils/helix/HelixHelper.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaGVsaXgvSGVsaXhIZWxwZXIuamF2YQ==) | `59.33% <86.48%> (+3.51%)` | :arrow_up: |
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `50.22% <100.00%> (-2.96%)` | :arrow_down: |
| [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [353 more](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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/7064?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/7064?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 [ac8f098...b54f511](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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 #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#issuecomment-861995023
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 [#7064](https://codecov.io/gh/apache/incubator-pinot/pull/7064?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f8cc5d5) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/ac8f098e8ff883abb8323391480039f3a5b8aab7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ac8f098) will **decrease** coverage by `31.88%`.
> The diff coverage is `86.48%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 #7064 +/- ##
=============================================
- Coverage 73.49% 41.60% -31.89%
+ Complexity 91 7 -84
=============================================
Files 1492 1493 +1
Lines 73422 73564 +142
Branches 10574 10594 +20
=============================================
- Hits 53960 30609 -23351
- Misses 15927 40369 +24442
+ Partials 3535 2586 -949
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `41.60% <86.48%> (+0.09%)` | :arrow_up: |
| unittests | `?` | |
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/7064?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../pinot/broker/broker/helix/HelixBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0hlbGl4QnJva2VyU3RhcnRlci5qYXZh) | `7.69% <0.00%> (+0.28%)` | :arrow_up: |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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=) | `0.00% <ø> (-30.36%)` | :arrow_down: |
| [...apache/pinot/controller/BaseControllerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9CYXNlQ29udHJvbGxlclN0YXJ0ZXIuamF2YQ==) | `77.44% <66.66%> (-4.41%)` | :arrow_down: |
| [...g/apache/pinot/common/utils/helix/HelixHelper.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaGVsaXgvSGVsaXhIZWxwZXIuamF2YQ==) | `48.59% <88.09%> (-7.22%)` | :arrow_down: |
| [.../pinot/server/starter/helix/BaseServerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9CYXNlU2VydmVyU3RhcnRlci5qYXZh) | `54.29% <88.88%> (-0.52%)` | :arrow_down: |
| [...e/pinot/broker/broker/helix/BaseBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jhc2VCcm9rZXJTdGFydGVyLmphdmE=) | `72.09% <90.90%> (-0.17%)` | :arrow_down: |
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `44.88% <100.00%> (-8.30%)` | :arrow_down: |
| [...ava/org/apache/pinot/minion/BaseMinionStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vQmFzZU1pbmlvblN0YXJ0ZXIuamF2YQ==) | `78.64% <100.00%> (+0.45%)` | :arrow_up: |
| [...c/main/java/org/apache/pinot/common/tier/Tier.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-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/7064/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [955 more](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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/7064?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/7064?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 [ac8f098...f8cc5d5](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] dongxiaoman commented on a change in pull request #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r659925421
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +498,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (defaultTags != null && !defaultTags.isEmpty()) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceId, instanceTags);
Review comment:
Good catch.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] dongxiaoman commented on a change in pull request #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r660199645
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager, String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager, InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+ helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()), instanceConfig),
+ "Failed to update instance config");
+ }
+
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String instanceId, String hostName, int hostPort) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ if (Strings.isNullOrEmpty(hostName)) {
+ LOGGER.info("host is empty, skip updating helix host.");
+ return false;
+ }
+ boolean updated = false;
+ // Update host and port if needed
+ if (!String.valueOf(hostPort).equals(instanceConfig.getPort())) {
+ instanceConfig.setPort(String.valueOf(hostPort));
+ updated = true;
+ }
+ if (!hostName.equals(instanceConfig.getHostName())) {
+ instanceConfig.setHostName(hostName);
+ updated = true;
+ }
+ updateInstanceConfig(helixManager, instanceConfig);
+ return updated;
+ }
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String instanceId, String hostName, String hostPort) {
+ if (Strings.isNullOrEmpty(hostPort)) {
Review comment:
Right, all changed to `log.warn` which makes more sense
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] dongxiaoman commented on a change in pull request #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r659221981
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -139,12 +139,13 @@ public HelixBrokerStarter(PinotConfiguration brokerConf) throws Exception {
if (brokerHost == null) {
brokerHost = _brokerConf.getProperty(Helix.SET_INSTANCE_ID_TO_HOSTNAME_KEY, false) ? NetUtils
.getHostnameOrAddress() : NetUtils.getHostAddress();
+ brokerConf.setProperty(Broker.CONFIG_OF_BROKER_HOSTNAME, brokerHost);
}
_brokerId = _brokerConf.getProperty(Helix.Instance.INSTANCE_ID_KEY,
- Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + _listenerConfigs.get(0).getPort());
+ Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + _listenerConfigs.get(0).getPort());
Review comment:
Done
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -318,7 +319,22 @@ public void start()
.registerMessageHandlerFactory(Message.MessageType.USER_DEFINE_MSG.toString(),
new BrokerUserDefinedMessageHandlerFactory(_routingManager, queryQuotaManager));
_participantHelixManager.connect();
- addInstanceTagIfNeeded();
+
+ HelixHelper.updateInstanceConfigIfNeeded(
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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 #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r660158809
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
Review comment:
`instanceTags` won't be `null` here
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
Review comment:
`getDefaultTags` should never be `null`
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager, String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager, InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+ helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()), instanceConfig),
+ "Failed to update instance config");
+ }
+
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String instanceId, String hostName, int hostPort) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ if (Strings.isNullOrEmpty(hostName)) {
+ LOGGER.info("host is empty, skip updating helix host.");
+ return false;
+ }
+ boolean updated = false;
+ // Update host and port if needed
+ if (!String.valueOf(hostPort).equals(instanceConfig.getPort())) {
+ instanceConfig.setPort(String.valueOf(hostPort));
+ updated = true;
+ }
+ if (!hostName.equals(instanceConfig.getHostName())) {
+ instanceConfig.setHostName(hostName);
+ updated = true;
+ }
+ updateInstanceConfig(helixManager, instanceConfig);
+ return updated;
+ }
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String instanceId, String hostName, String hostPort) {
+ if (Strings.isNullOrEmpty(hostPort)) {
Review comment:
Same here. Also why do we log error here, while log info in the other method?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
Review comment:
Pass in an `InstanceConfig` and a tag supplier, and return a boolean denoting whether the config is changed. We don't want to read an instance config and post it back for every update. We should only read the instance config once, track whether it is updated, and only post it back once if there are changes.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager, String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager, InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+ helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()), instanceConfig),
+ "Failed to update instance config");
+ }
+
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String instanceId, String hostName, int hostPort) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ if (Strings.isNullOrEmpty(hostName)) {
Review comment:
Can `hostName` be `null` or empty here? If so, should we skip updating the port as well?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager, String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager, InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+ helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()), instanceConfig),
+ "Failed to update instance config");
Review comment:
Log the instance id in the exception
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
Review comment:
`instanceTags` is always empty. We should log `defaultTags` instead
```suggestion
LOGGER.info("Updating instance: {} with default tags: {}", instanceId, defaultTags);
```
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager, String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager, InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+ helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()), instanceConfig),
+ "Failed to update instance config");
+ }
+
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String instanceId, String hostName, int hostPort) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ if (Strings.isNullOrEmpty(hostName)) {
Review comment:
We usually use `org.apache.commons.lang3.StringUtils` for string util functions
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager, String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager, InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+ helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()), instanceConfig),
+ "Failed to update instance config");
+ }
+
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String instanceId, String hostName, int hostPort) {
Review comment:
Similar here, pass in the `InstanceConfig` instead of `helixManager` and `instanceId`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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 #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r660158809
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
Review comment:
`instanceTags` won't be `null` here
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
Review comment:
`getDefaultTags` should never be `null`
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager, String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager, InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+ helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()), instanceConfig),
+ "Failed to update instance config");
+ }
+
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String instanceId, String hostName, int hostPort) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ if (Strings.isNullOrEmpty(hostName)) {
+ LOGGER.info("host is empty, skip updating helix host.");
+ return false;
+ }
+ boolean updated = false;
+ // Update host and port if needed
+ if (!String.valueOf(hostPort).equals(instanceConfig.getPort())) {
+ instanceConfig.setPort(String.valueOf(hostPort));
+ updated = true;
+ }
+ if (!hostName.equals(instanceConfig.getHostName())) {
+ instanceConfig.setHostName(hostName);
+ updated = true;
+ }
+ updateInstanceConfig(helixManager, instanceConfig);
+ return updated;
+ }
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String instanceId, String hostName, String hostPort) {
+ if (Strings.isNullOrEmpty(hostPort)) {
Review comment:
Same here. Also why do we log error here, while log info in the other method?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
Review comment:
Pass in an `InstanceConfig` and a tag supplier, and return a boolean denoting whether the config is changed. We don't want to read an instance config and post it back for every update. We should only read the instance config once, track whether it is updated, and only post it back once if there are changes.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager, String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager, InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+ helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()), instanceConfig),
+ "Failed to update instance config");
+ }
+
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String instanceId, String hostName, int hostPort) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ if (Strings.isNullOrEmpty(hostName)) {
Review comment:
Can `hostName` be `null` or empty here? If so, should we skip updating the port as well?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager, String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager, InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+ helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()), instanceConfig),
+ "Failed to update instance config");
Review comment:
Log the instance id in the exception
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
Review comment:
`instanceTags` is always empty. We should log `defaultTags` instead
```suggestion
LOGGER.info("Updating instance: {} with default tags: {}", instanceId, defaultTags);
```
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager, String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager, InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+ helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()), instanceConfig),
+ "Failed to update instance config");
+ }
+
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String instanceId, String hostName, int hostPort) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ if (Strings.isNullOrEmpty(hostName)) {
Review comment:
We usually use `org.apache.commons.lang3.StringUtils` for string util functions
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager, String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager, InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+ helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()), instanceConfig),
+ "Failed to update instance config");
+ }
+
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String instanceId, String hostName, int hostPort) {
Review comment:
Similar here, pass in the `InstanceConfig` instead of `helixManager` and `instanceId`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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 #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#issuecomment-861995023
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 [#7064](https://codecov.io/gh/apache/incubator-pinot/pull/7064?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0bcd93a) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/ac8f098e8ff883abb8323391480039f3a5b8aab7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ac8f098) will **decrease** coverage by `31.75%`.
> The diff coverage is `80.28%`.
> :exclamation: Current head 0bcd93a differs from pull request most recent head b69a5fb. Consider uploading reports for the commit b69a5fb to get more accurate results
[](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 #7064 +/- ##
=============================================
- Coverage 73.49% 41.74% -31.76%
+ Complexity 91 7 -84
=============================================
Files 1492 1492
Lines 73422 73444 +22
Branches 10574 10574
=============================================
- Hits 53960 30657 -23303
- Misses 15927 40210 +24283
+ Partials 3535 2577 -958
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `41.74% <80.28%> (+0.22%)` | :arrow_up: |
| unittests | `?` | |
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/7064?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../pinot/broker/broker/helix/HelixBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0hlbGl4QnJva2VyU3RhcnRlci5qYXZh) | `7.69% <0.00%> (+0.28%)` | :arrow_up: |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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=) | `0.00% <ø> (-30.36%)` | :arrow_down: |
| [...apache/pinot/controller/BaseControllerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9CYXNlQ29udHJvbGxlclN0YXJ0ZXIuamF2YQ==) | `77.44% <66.66%> (-4.41%)` | :arrow_down: |
| [...g/apache/pinot/common/utils/helix/HelixHelper.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaGVsaXgvSGVsaXhIZWxwZXIuamF2YQ==) | `45.45% <75.67%> (-10.36%)` | :arrow_down: |
| [.../pinot/server/starter/helix/BaseServerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9CYXNlU2VydmVyU3RhcnRlci5qYXZh) | `54.43% <90.00%> (-0.38%)` | :arrow_down: |
| [...e/pinot/broker/broker/helix/BaseBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jhc2VCcm9rZXJTdGFydGVyLmphdmE=) | `72.25% <90.90%> (ø)` | |
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `45.70% <100.00%> (-7.49%)` | :arrow_down: |
| [...ava/org/apache/pinot/minion/BaseMinionStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vQmFzZU1pbmlvblN0YXJ0ZXIuamF2YQ==) | `79.24% <100.00%> (+1.06%)` | :arrow_up: |
| [...c/main/java/org/apache/pinot/common/tier/Tier.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-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/7064/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [949 more](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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/7064?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/7064?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 [ac8f098...b69a5fb](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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 #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#issuecomment-861995023
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 [#7064](https://codecov.io/gh/apache/incubator-pinot/pull/7064?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b303ae4) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/0a020b4558c815490b01516fb41556e6f9420406?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0a020b4) will **decrease** coverage by `8.02%`.
> The diff coverage is `21.21%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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 #7064 +/- ##
============================================
- Coverage 73.59% 65.56% -8.03%
Complexity 91 91
============================================
Files 1477 1477
Lines 72800 72832 +32
Branches 10471 10475 +4
============================================
- Hits 53576 47752 -5824
- Misses 15761 21728 +5967
+ Partials 3463 3352 -111
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `?` | |
| unittests | `65.56% <21.21%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/7064?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...g/apache/pinot/common/utils/helix/HelixHelper.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaGVsaXgvSGVsaXhIZWxwZXIuamF2YQ==) | `49.44% <0.00%> (-6.37%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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=) | `29.82% <ø> (ø)` | |
| [...org/apache/pinot/controller/ControllerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyU3RhcnRlci5qYXZh) | `67.67% <25.00%> (-6.71%)` | :arrow_down: |
| [.../pinot/broker/broker/helix/HelixBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0hlbGl4QnJva2VyU3RhcnRlci5qYXZh) | `60.89% <27.27%> (-7.70%)` | :arrow_down: |
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `50.45% <50.00%> (-3.22%)` | :arrow_down: |
| [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/incubator-pinot/pull/7064/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [348 more](https://codecov.io/gh/apache/incubator-pinot/pull/7064/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/7064?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/7064?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 [0a020b4...b303ae4](https://codecov.io/gh/apache/incubator-pinot/pull/7064?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] codecov-commenter edited a comment on pull request #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#issuecomment-861995023
--
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] dongxiaoman commented on a change in pull request #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r659221853
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -139,12 +139,13 @@ public HelixBrokerStarter(PinotConfiguration brokerConf) throws Exception {
if (brokerHost == null) {
brokerHost = _brokerConf.getProperty(Helix.SET_INSTANCE_ID_TO_HOSTNAME_KEY, false) ? NetUtils
.getHostnameOrAddress() : NetUtils.getHostAddress();
+ brokerConf.setProperty(Broker.CONFIG_OF_BROKER_HOSTNAME, brokerHost);
Review comment:
Done
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -266,65 +264,21 @@ private void registerServiceStatusHandler() {
}
private void updateInstanceConfigIfNeeded(String host, int port) {
- InstanceConfig instanceConfig = _helixAdmin.getInstanceConfig(_helixClusterName, _instanceId);
- boolean needToUpdateInstanceConfig = false;
-
- // Add default instance tags if not exist
- List<String> instanceTags = instanceConfig.getTags();
- if (instanceTags == null || instanceTags.size() == 0) {
- if (ZKMetadataProvider.getClusterTenantIsolationEnabled(_helixManager.getHelixPropertyStore())) {
- instanceConfig.addTag(TagNameUtils.getOfflineTagForTenant(null));
- instanceConfig.addTag(TagNameUtils.getRealtimeTagForTenant(null));
- } else {
- instanceConfig.addTag(Helix.UNTAGGED_SERVER_INSTANCE);
- }
- needToUpdateInstanceConfig = true;
- }
-
- // Update host and port if needed
- if (!host.equals(instanceConfig.getHostName())) {
- instanceConfig.setHostName(host);
- needToUpdateInstanceConfig = true;
- }
- String portStr = Integer.toString(port);
- if (!portStr.equals(instanceConfig.getPort())) {
- instanceConfig.setPort(portStr);
- needToUpdateInstanceConfig = true;
- }
-
- // Update instance config with environment properties
- if (_pinotEnvironmentProvider != null) {
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] dongxiaoman commented on a change in pull request #7064: Allow updating controller and broker helix hostname
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r659915596
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -53,9 +54,7 @@ public HelixBrokerStarter(PinotConfiguration brokerConf, String clusterName, Str
private static PinotConfiguration applyBrokerConfigs(PinotConfiguration brokerConf, String clusterName, String zkServers, @Nullable String brokerHost) {
brokerConf.setProperty(Helix.CONFIG_OF_CLUSTER_NAME, clusterName);
brokerConf.setProperty(Helix.CONFIG_OF_ZOOKEEPR_SERVER, zkServers);
- if (brokerHost == null) {
- brokerConf.clearProperty(Broker.CONFIG_OF_BROKER_HOSTNAME);
- } else {
+ if (!Strings.isNullOrEmpty(brokerHost)) {
Review comment:
Based on code reading, I don't see a use case that we need to clear the values. cc @Jackie-Jiang for double checking
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +498,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (defaultTags != null && !defaultTags.isEmpty()) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceId, instanceTags);
Review comment:
Good catch.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
Review comment:
Changed to `.isEmpty()`
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
Review comment:
All updated.
Now the tracking logic is also in HelixHelper so the code for 4 host types are shared.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager, String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager, InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+ helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()), instanceConfig),
+ "Failed to update instance config");
Review comment:
Added.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager, String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager, InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+ helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()), instanceConfig),
+ "Failed to update instance config");
+ }
+
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String instanceId, String hostName, int hostPort) {
Review comment:
Changed
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager, String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager, InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+ helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()), instanceConfig),
+ "Failed to update instance config");
+ }
+
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String instanceId, String hostName, int hostPort) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ if (Strings.isNullOrEmpty(hostName)) {
Review comment:
Changed to use `org.apache.commons.lang3.StringUtils`; and `hostName` with `port` are both tested
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager, String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager, InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+ helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()), instanceConfig),
+ "Failed to update instance config");
+ }
+
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String instanceId, String hostName, int hostPort) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ if (Strings.isNullOrEmpty(hostName)) {
+ LOGGER.info("host is empty, skip updating helix host.");
+ return false;
+ }
+ boolean updated = false;
+ // Update host and port if needed
+ if (!String.valueOf(hostPort).equals(instanceConfig.getPort())) {
+ instanceConfig.setPort(String.valueOf(hostPort));
+ updated = true;
+ }
+ if (!hostName.equals(instanceConfig.getHostName())) {
+ instanceConfig.setHostName(hostName);
+ updated = true;
+ }
+ updateInstanceConfig(helixManager, instanceConfig);
+ return updated;
+ }
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String instanceId, String hostName, String hostPort) {
+ if (Strings.isNullOrEmpty(hostPort)) {
Review comment:
Right, all changed to `log.warn` which makes more sense
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}", instanceTags, instanceId);
Review comment:
Changed too.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>> getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager, instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null : getDefaultTags.get();
Review comment:
Added `NonNull` annotation and call directly.
If no tags, we can call something like `ImmutableList::of` directly
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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