Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add session property #156

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

add session property #156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions charts/trino/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ The following table lists the configurable parameters of the Trino chart and the
| `server.autoscaling.behavior` | | `{}` |
| `accessControl` | | `{}` |
| `resourceGroups` | | `{}` |
| `sessionPropertyConfig` | | `{}` |
| `additionalNodeProperties` | | `{}` |
| `additionalConfigProperties` | | `{}` |
| `additionalLogProperties` | | `{}` |
Expand Down
21 changes: 21 additions & 0 deletions charts/trino/templates/configmap-coordinator.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,12 @@ data:
resource-groups.config-file={{ .Values.server.config.path }}/resource-groups/resource-groups.json
{{- end }}

{{- if .Values.sessionPropertyConfig }}
session-property-config.properties: |
session-property-config.configuration-manager=file
session-property-manager.config-file={{ .Values.server.config.path }}/session-property-config/session-property-config.json
{{- end }}

exchange-manager.properties: |
exchange-manager.name={{ .Values.server.exchangeManager.name }}
{{ if eq .Values.server.exchangeManager.name "filesystem" }}
Expand Down Expand Up @@ -160,6 +166,21 @@ data:

---

{{- if .Values.sessionPropertyConfig }}
apiVersion: v1
kind: ConfigMap
metadata:
name: trino-session-property-config-volume-coordinator
Copy link
Member

@huw0 huw0 May 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure historically as to why some config is deployed to a new configmap while everything else (node.properties, jvm.config etc) is deployed into a single configmap?

However if a separate configmap is still desired, please avoid hardcoding the name and instead make use of a template function? See the naming used for the main configmap and the example in #170 and #109 .

It is helpful (for testing etc) to be able to ensure that multiple trino instances can be deployed into a single namespace.

labels:
{{- include "trino.labels" . | nindent 4 }}
app.kubernetes.io/component: coordinator
data:
session-property-config.json: |-
{{- .Values.sessionPropertyConfig.sessionPropertyConfig | nindent 4 }}
{{- end }}

---

apiVersion: v1
kind: ConfigMap
metadata:
Expand Down
9 changes: 9 additions & 0 deletions charts/trino/templates/deployment-coordinator.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@ spec:
configMap:
name: trino-resource-groups-volume-coordinator
{{- end }}
{{- if .Values.sessionPropertyConfig }}
- name: session-property-config-volume
configMap:
name: trino-session-property-config-volume-coordinator
{{- end }}
{{- if or .Values.auth.passwordAuth .Values.auth.passwordAuthSecret .Values.auth.groups }}
- name: file-authentication-volume
secret:
Expand Down Expand Up @@ -126,6 +131,10 @@ spec:
- mountPath: {{ .Values.server.config.path }}/resource-groups
name: resource-groups-volume
{{- end }}
{{- if .Values.sessionPropertyConfig }}
- mountPath: {{ .Values.server.config.path }}/session-property-config
name: session-property-config-volume
{{- end }}
{{- range .Values.secretMounts }}
- name: {{ .name }}
mountPath: {{ .path }}
Expand Down
25 changes: 25 additions & 0 deletions charts/trino/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,31 @@ resourceGroups: {}
# ]
# }

sessionPropertyConfig: {}
Copy link
Member

Choose a reason for hiding this comment

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

@joshuasu-async sorry for the delay. I recently added a more complete test in https://github.com/trinodb/charts/blob/main/test-values.yaml. Can you update it to enable this config so we know this works correctly?

Also, please rebase and see the updated way of formatting examples here. We now include them in the README, so they should be formatted using Markdown.

# # Session property config file is mounted to /etc/trino/session-property-config/session-property-config.json
# sessionPropertyConfig: |-
# [
# {
# "group": "global.admin",
# "sessionProperties": {
# "query_max_execution_time": "5h",
# "scale_writers": "true"
# }
# },
# {
# "group": "global.finance_human_resources",
# "sessionProperties": {
# "query_max_execution_time": "3h"
# }
# },
# {
# "group": "global.general",
# "sessionProperties": {
# "query_max_scan_physical_bytes": "1GB",
# "query_max_cpu_time": "10m"
# }
# }
# ]

additionalNodeProperties: {}

Expand Down
Loading