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/05/26 17:00:02 UTC
[GitHub] [incubator-pinot] ChethanUK opened a new pull request #6985: Replace gcp hardcoded values with generic annotations
ChethanUK opened a new pull request #6985:
URL: https://github.com/apache/incubator-pinot/pull/6985
## Description
Instead of hardcoded GCP load balancer values replacing them with generic annotation map.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] daniellavoie commented on a change in pull request #6985: Replace gcp hardcoded values with generic annotations
Posted by GitBox <gi...@apache.org>.
daniellavoie commented on a change in pull request #6985:
URL: https://github.com/apache/incubator-pinot/pull/6985#discussion_r639967215
##########
File path: kubernetes/helm/pinot/templates/broker/service-external.yaml
##########
@@ -24,10 +24,7 @@ kind: Service
metadata:
name: {{ template "pinot.broker.external" . }}
annotations:
- type: "service-external"
- {{- if .Values.broker.service.gcpInternalLB }}
- cloud.google.com/load-balancer-type: Internal
- {{- end }}
+{{ toYaml .Values.controller.serviceAnnotationsExternal | indent 4 }}
Review comment:
`.Values.broker.external.annotations`
##########
File path: kubernetes/helm/pinot/templates/controller/service-external.yaml
##########
@@ -24,10 +24,7 @@ kind: Service
metadata:
name: {{ template "pinot.controller.external" . }}
annotations:
- type: "service-external"
- {{- if .Values.broker.service.gcpInternalLB }}
- cloud.google.com/load-balancer-type: Internal
- {{- end }}
+{{ toYaml .Values.controller.serviceAnnotationsExternal | indent 4 }}
Review comment:
`.Values.controller.external.annotations`
##########
File path: kubernetes/helm/pinot/templates/server/service.yaml
##########
@@ -22,10 +22,7 @@ kind: Service
metadata:
name: {{ include "pinot.server.fullname" . }}
annotations:
- type: "service"
- {{- if .Values.server.service.gcpInternalLB }}
- cloud.google.com/load-balancer-type: Internal
- {{- end }}
+{{ toYaml .Values.controller.serviceAnnotations | indent 4 }}
Review comment:
`.Values.server.service.annotations`
##########
File path: kubernetes/helm/pinot/templates/minion/service.yaml
##########
@@ -22,10 +22,7 @@ kind: Service
metadata:
name: {{ include "pinot.minion.fullname" . }}
annotations:
- type: "service"
- {{- if .Values.minion.service.gcpInternalLB }}
- cloud.google.com/load-balancer-type: Internal
- {{- end }}
+{{ toYaml .Values.controller.serviceAnnotations | indent 4 }}
Review comment:
`.Values.minion.service.annotations`
##########
File path: kubernetes/helm/pinot/values.yaml
##########
@@ -117,6 +115,14 @@ controller:
podAnnotations: {}
+ # For example, in private GKE cluster, you might add cloud.google.com/load-balancer-type: Internal
+ serviceAnnotations:
+ type: "service"
Review comment:
`controller.service.annotations` already service that purpose. Also `type: "service"` and should not be present by default.
##########
File path: kubernetes/helm/pinot/templates/broker/service.yaml
##########
@@ -22,10 +22,7 @@ kind: Service
metadata:
name: {{ include "pinot.broker.fullname" . }}
annotations:
- type: "service"
- {{- if .Values.broker.service.gcpInternalLB }}
- cloud.google.com/load-balancer-type: Internal
- {{- end }}
+{{ toYaml .Values.controller.serviceAnnotations | indent 4 }}
Review comment:
`.Values.broker.service.annotations`
##########
File path: kubernetes/helm/pinot/values.yaml
##########
@@ -98,14 +98,12 @@ controller:
port: 9000
nodePort: ""
protocol: TCP
- gcpInternalLB: false
name: controller
external:
enabled: true
Review comment:
Add `annotations: {}` here.
##########
File path: kubernetes/helm/pinot/templates/controller/service.yaml
##########
@@ -22,10 +22,7 @@ kind: Service
metadata:
name: {{ include "pinot.controller.fullname" . }}
annotations:
- type: "service"
- {{- if .Values.controller.service.gcpInternalLB }}
- cloud.google.com/load-balancer-type: Internal
- {{- end }}
+{{ toYaml .Values.controller.serviceAnnotations | indent 4 }}
Review comment:
`.Values.controller.service.annotations`
##########
File path: kubernetes/helm/pinot/values.yaml
##########
@@ -275,6 +286,10 @@ server:
podAnnotations: {}
+ # For example, in private GKE cluster, you might add cloud.google.com/load-balancer-type: Internal
+ serviceAnnotations:
+ type: "service"
+
Review comment:
Same observations as the controller configs.
##########
File path: kubernetes/helm/pinot/values.yaml
##########
@@ -117,6 +115,14 @@ controller:
podAnnotations: {}
+ # For example, in private GKE cluster, you might add cloud.google.com/load-balancer-type: Internal
+ serviceAnnotations:
+ type: "service"
+
+ # serviceAnnotationsExternal: {}
+ serviceAnnotationsExternal:
Review comment:
Use `.Values.controller.external.annotations` instead. `type: "service-external"` has no effect so no need to provide that by default.
##########
File path: kubernetes/helm/pinot/values.yaml
##########
@@ -194,6 +198,14 @@ broker:
podAnnotations: {}
+ # For example, in private GKE cluster, you might add cloud.google.com/load-balancer-type: Internal
+ serviceAnnotations:
+ type: "service"
+
+ # serviceAnnotationsExternal: {}
+ serviceAnnotationsExternal:
+ type: "service-external"
+
Review comment:
Same observations as the controller configs.
##########
File path: kubernetes/helm/pinot/values.yaml
##########
@@ -353,6 +367,10 @@ minion:
podAnnotations: {}
+ # For example, in private GKE cluster, you might add cloud.google.com/load-balancer-type: Internal
+ serviceAnnotations:
+ type: "service"
+
Review comment:
Same observations as the controller configs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-commenter commented on pull request #6985: Replace gcp hardcoded values with generic annotations
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #6985:
URL: https://github.com/apache/incubator-pinot/pull/6985#issuecomment-849063649
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6985?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 [#6985](https://codecov.io/gh/apache/incubator-pinot/pull/6985?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (61e6603) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/b8a92c41e3d063c436e37965ade577ff8e2b5f86?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b8a92c4) will **decrease** coverage by `7.89%`.
> The diff coverage is `n/a`.
> :exclamation: Current head 61e6603 differs from pull request most recent head 6f11019. Consider uploading reports for the commit 6f11019 to get more accurate results
[](https://codecov.io/gh/apache/incubator-pinot/pull/6985?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 #6985 +/- ##
============================================
- Coverage 73.42% 65.53% -7.90%
Complexity 12 12
============================================
Files 1441 1441
Lines 71431 71431
Branches 10351 10351
============================================
- Hits 52446 46809 -5637
- Misses 15464 21234 +5770
+ Partials 3521 3388 -133
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `?` | |
| unittests | `65.53% <ø> (-0.02%)` | :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/6985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6985/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/6985/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/6985/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/6985/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/6985/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...t/core/startree/plan/StarTreeDocIdSetPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6985/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlRG9jSWRTZXRQbGFuTm9kZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../core/startree/plan/StarTreeTransformPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6985/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlVHJhbnNmb3JtUGxhbk5vZGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...core/startree/plan/StarTreeProjectionPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6985/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlUHJvamVjdGlvblBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...t/minion/executor/MinionTaskZkMetadataManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/6985/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhlY3V0b3IvTWluaW9uVGFza1prTWV0YWRhdGFNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...startree/executor/StarTreeAggregationExecutor.java](https://codecov.io/gh/apache/incubator-pinot/pull/6985/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9leGVjdXRvci9TdGFyVHJlZUFnZ3JlZ2F0aW9uRXhlY3V0b3IuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [337 more](https://codecov.io/gh/apache/incubator-pinot/pull/6985/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/6985?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/6985?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 [b8a92c4...6f11019](https://codecov.io/gh/apache/incubator-pinot/pull/6985?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] ChethanUK commented on a change in pull request #6985: Replace gcp hardcoded values with generic annotations
Posted by GitBox <gi...@apache.org>.
ChethanUK commented on a change in pull request #6985:
URL: https://github.com/apache/incubator-pinot/pull/6985#discussion_r639989738
##########
File path: kubernetes/helm/pinot/templates/broker/service-external.yaml
##########
@@ -24,10 +24,7 @@ kind: Service
metadata:
name: {{ template "pinot.broker.external" . }}
annotations:
- type: "service-external"
- {{- if .Values.broker.service.gcpInternalLB }}
- cloud.google.com/load-balancer-type: Internal
- {{- end }}
+{{ toYaml .Values.controller.serviceAnnotationsExternal | indent 4 }}
Review comment:
Copy-paste errors. Fixing.
--
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] ChethanUK commented on a change in pull request #6985: Replace gcp hardcoded values with generic annotations
Posted by GitBox <gi...@apache.org>.
ChethanUK commented on a change in pull request #6985:
URL: https://github.com/apache/incubator-pinot/pull/6985#discussion_r639989738
##########
File path: kubernetes/helm/pinot/templates/broker/service-external.yaml
##########
@@ -24,10 +24,7 @@ kind: Service
metadata:
name: {{ template "pinot.broker.external" . }}
annotations:
- type: "service-external"
- {{- if .Values.broker.service.gcpInternalLB }}
- cloud.google.com/load-balancer-type: Internal
- {{- end }}
+{{ toYaml .Values.controller.serviceAnnotationsExternal | indent 4 }}
Review comment:
Copy-paste errors. Fixing.
>Finally, annotations.type: "service" does nothing on Kubernetes so I wouldn't add that as a default value.
Ya agree, Just refactored. Removing
--
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