Skip to content

Support LB spec & coordinator-only annotations #369

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jirislav
Copy link
Contributor

Resolves #366

@cla-bot cla-bot bot added the cla-signed label Jul 18, 2025
@jirislav jirislav force-pushed the coordinator-specific-annotations-and-supporting-generic-service-attributes branch from fbcd41b to c536f4b Compare July 18, 2025 07:47
@jirislav jirislav force-pushed the coordinator-specific-annotations-and-supporting-generic-service-attributes branch from c536f4b to 3ae4992 Compare July 18, 2025 08:21
@@ -550,6 +550,21 @@ secretMounts: []
# ```

coordinator:
service: {}
# coordinator.service -- Service overrides just for the coordinator service. It is merged with the root-level service specification. Beware that specifying spec.ports will override the dynamically generated ports for JMX exporter as well as the additionalExposedPorts.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, we should not allow specifying both the top-level service and coordinator or worker services. It's too easy to make a mistake, like specifying the wrong type, and one will be ignored.

There's a similar PR #360 and we had this discussion there: #360 (comment)

@@ -1,4 +1,7 @@
{{- $workerJmx := merge .Values.jmx.worker (omit .Values.jmx "coordinator" "worker") -}}
{{- $workerSvcSpecOverride := index .Values.worker.service "spec" | default dict -}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This template gets pretty complicated, and we need to update tests to cover at least parts of the new use cases supported. See if you could update tests/trino/test-values.yaml. If not, create a new test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

It's not possible to provide LoadBalancer spec or annotations just to coordinator
2 participants