-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HELM] Fix Kubernetes Routing Issue #13450
[HELM] Fix Kubernetes Routing Issue #13450
Conversation
Does it mean that with this change, pods are going to get the request before they get into Ready Stage? |
With this change, the DNS for each server will always resolve irrespective of whether the pod is in "Ready" state or not. Pinot will however only make requests to the server when it is online as per Zookeeper. |
Thanks for the explanation. I just checked, and the zookeeper is doing the same for the headless service. |
Change to just headless services is sufficient. This has been tested on our cluster. If you agree, I can remove this change from "headful" services. |
In my opinion, it should be only in headless svc, but I suggest we wait for others' input as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -29,6 +29,7 @@ metadata: | |||
{{- include "pinot.brokerLabels" . | nindent 4 }} | |||
spec: | |||
type: {{ .Values.broker.external.type }} | |||
publishNotReadyAddresses: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's make default external to false here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
I think we should only make default value for service-headless and server service to true, all others to false. |
Pinot handles all the internal readiness for routing request to servers, so it's ok to expose all the server/minion ips once the pod is up but not ready. |
@@ -27,6 +27,7 @@ metadata: | |||
{{- include "pinot.brokerLabels" . | nindent 4 }} | |||
spec: | |||
type: {{ .Values.broker.service.type }} | |||
publishNotReadyAddresses: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default to false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -29,6 +29,7 @@ metadata: | |||
{{- include "pinot.controllerLabels" . | nindent 4 }} | |||
spec: | |||
type: {{ .Values.controller.external.type }} | |||
publishNotReadyAddresses: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default to false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -27,6 +27,7 @@ metadata: | |||
{{- include "pinot.controllerLabels" . | nindent 4 }} | |||
spec: | |||
type: {{ .Values.controller.service.type }} | |||
publishNotReadyAddresses: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default to false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #13450 +/- ##
============================================
+ Coverage 61.75% 64.00% +2.25%
- Complexity 207 1539 +1332
============================================
Files 2436 2596 +160
Lines 133233 143295 +10062
Branches 20636 21948 +1312
============================================
+ Hits 82274 91718 +9444
+ Misses 44911 44833 -78
- Partials 6048 6744 +696
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@xiangfu0 Can we merge this? |
Yes please, can we merge this, otherwise we might have to fork the chart. |
This PR aims to partially solve the issue mentioned here
With publishNotReadyAddresses set to true in service resource, kubernetes will publish the DNS address irrespective of whether the pod is in Ready state or not. Then the sole responsibility of when to start making requests to the newly restarted server/broker lies on Pinot.
The following error will be fixed with this PR