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 2022/05/10 05:25:47 UTC
[GitHub] [pinot] saurabhd336 opened a new pull request, #8669: Change minion's default port, enable minion health and readiness probe via helm chart
saurabhd336 opened a new pull request, #8669:
URL: https://github.com/apache/pinot/pull/8669
This PR adds support for minion's health and readiness probe when deploying using helm charts
--
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] [pinot] saurabhd336 commented on a diff in pull request #8669: Change minion's default port, enable minion health and readiness probe via helm chart
Posted by GitBox <gi...@apache.org>.
saurabhd336 commented on code in PR #8669:
URL: https://github.com/apache/pinot/pull/8669#discussion_r869915450
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -527,7 +527,7 @@ public static class Minion {
public static final String CONFIG_OF_TASK_AUTH_TOKEN = "task.auth.token";
public static final String CONFIG_OF_ADMIN_API_PORT = "pinot.minion.adminapi.port";
public static final String MINION_TLS_PREFIX = "pinot.minion.tls";
- public static final int DEFAULT_ADMIN_API_PORT = 6500;
+ public static final int DEFAULT_ADMIN_API_PORT = 9514;
Review Comment:
Ack.
--
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] [pinot] saurabhd336 commented on a diff in pull request #8669: Change minion's default port, enable minion health and readiness probe via helm chart
Posted by GitBox <gi...@apache.org>.
saurabhd336 commented on code in PR #8669:
URL: https://github.com/apache/pinot/pull/8669#discussion_r870956874
##########
pinot-tools/src/main/resources/conf/pinot-minion.conf:
##########
@@ -33,7 +33,7 @@ pinot.set.instance.id.to.hostname=true
# pinot.minion.host=localhost
# Pinot Minion Port
-pinot.minion.port=8098
+pinot.minion.port=9154
Review Comment:
Ack
##########
pinot-tools/src/main/resources/conf/pinot-minion.conf:
##########
@@ -33,7 +33,7 @@ pinot.set.instance.id.to.hostname=true
# pinot.minion.host=localhost
# Pinot Minion Port
-pinot.minion.port=8098
+pinot.minion.port=9154
# Pinot Minion Admin API Port
pinot.minion.adminapi.port=6500
Review Comment:
Ack
--
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] [pinot] Jackie-Jiang commented on a diff in pull request #8669: Change minion's default port, enable minion health and readiness probe via helm chart
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8669:
URL: https://github.com/apache/pinot/pull/8669#discussion_r869491638
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -527,7 +527,7 @@ public static class Minion {
public static final String CONFIG_OF_TASK_AUTH_TOKEN = "task.auth.token";
public static final String CONFIG_OF_ADMIN_API_PORT = "pinot.minion.adminapi.port";
public static final String MINION_TLS_PREFIX = "pinot.minion.tls";
- public static final int DEFAULT_ADMIN_API_PORT = 6500;
+ public static final int DEFAULT_ADMIN_API_PORT = 9514;
Review Comment:
We should remove this config completely. We don't need a separate admin port for minion, but use the regular port (similar to controller and broker. Server is slightly different and has both query port and admin port).
You may take a look at the `ListenerConfigUtil` and modify the `buildMinionAdminConfigs()` to be similar to `buildControllerConfigs()`
--
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] [pinot] codecov-commenter commented on pull request #8669: Change minion's default port, enable minion health and readiness probe via helm chart
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8669:
URL: https://github.com/apache/pinot/pull/8669#issuecomment-1123258011
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8669?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 [#8669](https://codecov.io/gh/apache/pinot/pull/8669?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (89dbb79) into [master](https://codecov.io/gh/apache/pinot/commit/a12b760b74b7ac9e1b30a8fe990e6c30a35e1275?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a12b760) will **decrease** coverage by `54.35%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #8669 +/- ##
=============================================
- Coverage 68.64% 14.29% -54.36%
+ Complexity 4472 84 -4388
=============================================
Files 1696 1651 -45
Lines 88565 86630 -1935
Branches 13271 13058 -213
=============================================
- Hits 60795 12381 -48414
- Misses 23551 73339 +49788
+ Partials 4219 910 -3309
```
| Flag | Coverage Δ | |
|---|---|---|
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `14.29% <0.00%> (+<0.01%)` | :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/pinot/pull/8669?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...org/apache/pinot/core/util/ListenerConfigUtil.java](https://codecov.io/gh/apache/pinot/pull/8669/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0xpc3RlbmVyQ29uZmlnVXRpbC5qYXZh) | `0.00% <0.00%> (-76.20%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/pinot/pull/8669/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% <ø> (-21.32%)` | :arrow_down: |
| [...src/main/java/org/apache/pinot/sql/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/8669/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvRmlsdGVyS2luZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/8669/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/StringUtil.java](https://codecov.io/gh/apache/pinot/pull/8669/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvU3RyaW5nVXRpbC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8669/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/8669/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/8669/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/8669/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/NoOpRecording.java](https://codecov.io/gh/apache/pinot/pull/8669/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvTm9PcFJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1298 more](https://codecov.io/gh/apache/pinot/pull/8669/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/pinot/pull/8669?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/pinot/pull/8669?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 [a12b760...89dbb79](https://codecov.io/gh/apache/pinot/pull/8669?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] [pinot] Jackie-Jiang commented on a diff in pull request #8669: Change minion's default port, enable minion health and readiness probe via helm chart
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8669:
URL: https://github.com/apache/pinot/pull/8669#discussion_r870622462
##########
pinot-tools/src/main/resources/conf/pinot-minion.conf:
##########
@@ -33,7 +33,7 @@ pinot.set.instance.id.to.hostname=true
# pinot.minion.host=localhost
# Pinot Minion Port
-pinot.minion.port=8098
+pinot.minion.port=9154
Review Comment:
```suggestion
pinot.minion.port=9514
```
##########
pinot-tools/src/main/resources/conf/pinot-minion.conf:
##########
@@ -33,7 +33,7 @@ pinot.set.instance.id.to.hostname=true
# pinot.minion.host=localhost
# Pinot Minion Port
-pinot.minion.port=8098
+pinot.minion.port=9154
# Pinot Minion Admin API Port
pinot.minion.adminapi.port=6500
Review Comment:
Remove this part
##########
pinot-core/src/main/java/org/apache/pinot/core/util/ListenerConfigUtil.java:
##########
@@ -155,23 +155,14 @@ public static List<ListenerConfig> buildServerAdminConfigs(PinotConfiguration se
public static List<ListenerConfig> buildMinionAdminConfigs(PinotConfiguration minionConf) {
Review Comment:
(minor) to avoid confusion
```suggestion
public static List<ListenerConfig> buildMinionConfigs(PinotConfiguration minionConf) {
```
--
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] [pinot] saurabhd336 commented on pull request #8669: Change minion's default port, enable minion health and readiness probe via helm chart
Posted by GitBox <gi...@apache.org>.
saurabhd336 commented on PR #8669:
URL: https://github.com/apache/pinot/pull/8669#issuecomment-1121946332
@Jackie-Jiang @daniellavoie Please have a look
--
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] [pinot] saurabhd336 commented on a diff in pull request #8669: Change minion's default port, enable minion health and readiness probe via helm chart
Posted by GitBox <gi...@apache.org>.
saurabhd336 commented on code in PR #8669:
URL: https://github.com/apache/pinot/pull/8669#discussion_r870956736
##########
pinot-core/src/main/java/org/apache/pinot/core/util/ListenerConfigUtil.java:
##########
@@ -155,23 +155,14 @@ public static List<ListenerConfig> buildServerAdminConfigs(PinotConfiguration se
public static List<ListenerConfig> buildMinionAdminConfigs(PinotConfiguration minionConf) {
Review Comment:
Ack
--
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] [pinot] Jackie-Jiang merged pull request #8669: Change minion's default port, enable minion health and readiness probe via helm chart
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged PR #8669:
URL: https://github.com/apache/pinot/pull/8669
--
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