Skip to content

Conversation

@sudohainguyen
Copy link
Contributor

The MR is about to support passing file-based session property config and mount to config path which is default by /etc/trino

@cla-bot
Copy link

cla-bot bot commented May 12, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@sudohainguyen sudohainguyen force-pushed the add-session-property branch from 00e0d97 to 9de3d47 Compare May 12, 2025 15:08
@cla-bot
Copy link

cla-bot bot commented May 12, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@sudohainguyen sudohainguyen force-pushed the add-session-property branch from 9de3d47 to 6b5e665 Compare May 12, 2025 16:12
@cla-bot
Copy link

cla-bot bot commented May 12, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@sudohainguyen
Copy link
Contributor Author

hi @nineinchnick is there any. blockers?

@nineinchnick nineinchnick added the enhancement New feature or request label May 18, 2025
@nineinchnick
Copy link
Member

No, except for the signed CLA. They're processed every 2-3 weeks, and I'd expect the next batch to be processed in a few days.

@sudohainguyen sudohainguyen force-pushed the add-session-property branch from 6b5e665 to 1e7113a Compare May 20, 2025 01:43
@cla-bot
Copy link

cla-bot bot commented May 20, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@sudohainguyen sudohainguyen force-pushed the add-session-property branch from 1e7113a to 43a44d3 Compare May 20, 2025 13:08
@cla-bot
Copy link

cla-bot bot commented May 20, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@sudohainguyen sudohainguyen force-pushed the add-session-property branch from 43a44d3 to 9d3984f Compare May 20, 2025 13:46
@cla-bot
Copy link

cla-bot bot commented May 20, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@sudohainguyen sudohainguyen force-pushed the add-session-property branch 2 times, most recently from d3a875d to 8ac7c68 Compare May 20, 2025 14:23
@cla-bot
Copy link

cla-bot bot commented May 20, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@nineinchnick
Copy link
Member

@sudohainguyen you know you can run tests locally, right? :-)

@sudohainguyen
Copy link
Contributor Author

@nineinchnick my bad, been trying to run test script in local but got this

./test.sh: line 5: default: unbound variable

@nineinchnick
Copy link
Member

Which version of bash do you use? If you have 3.x, try upgrading to 5.x. Bash 3 is 21 years old.

@sudohainguyen
Copy link
Contributor Author

ah right, that's it! let me try

@sudohainguyen
Copy link
Contributor Author

sudohainguyen commented May 20, 2025

cannot upgrade directly in my mac, any other ways?
good now, running test

@sudohainguyen sudohainguyen force-pushed the add-session-property branch from 8ac7c68 to 432505b Compare May 20, 2025 15:33
@sudohainguyen
Copy link
Contributor Author

passed in local with the change but got pod trino-g3ayjhc6cq-trino-test-networkpolicy failed

@nineinchnick
Copy link
Member

@cla-bot check

@cla-bot cla-bot bot added the cla-signed label Jun 16, 2025
@cla-bot
Copy link

cla-bot bot commented Jun 16, 2025

The cla-bot has been summoned, and re-checked this pull request!

@sudohainguyen sudohainguyen force-pushed the add-session-property branch from 432505b to 406832d Compare July 12, 2025 06:43
@sudohainguyen sudohainguyen force-pushed the add-session-property branch 3 times, most recently from d662ed6 to 3352e10 Compare July 12, 2025 06:46
@nineinchnick nineinchnick requested a review from Copilot July 12, 2025 07:57
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for mounting Trino session property configurations via a file or properties, aligned with the official session property manager.

  • Introduce a sessionProperty key in Helm chart values with two modes (configmap or properties).
  • Update the coordinator ConfigMap template to generate session-property-config.properties and JSON based on the chosen mode.
  • Add examples in values.yaml, tests, and a README entry for the new sessionProperty configuration.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
tests/trino/test-values.yaml Added sessionProperty and sample session properties JSON; inserted system_session_properties.
charts/trino/values.yaml Defined sessionProperty in default values with commented examples for both modes.
charts/trino/templates/configmap-coordinator.yaml Render session-property configuration files when sessionProperty is set.
charts/trino/README.md Documented the new sessionProperty top-level value.
Comments suppressed due to low confidence (4)

charts/trino/templates/configmap-coordinator.yaml:120

  • The condition if .Values.sessionProperty is always true (defaults to {}), causing Helm to fail when no type is set. Change the guard to check for the presence of .Values.sessionProperty.type instead.
{{- if .Values.sessionProperty }}

tests/trino/test-values.yaml:349

  • This duplicate sessionProperties block does not match the chart's expected sessionProperty schema. Rename or remove it to keep test values aligned with the Helm chart values.
sessionProperties: |-

charts/trino/README.md:352

  • [nitpick] Expand this entry to document the sessionProperty sub-fields (type, sessionPropertiesConfig, properties) and include usage examples for both modes.
* `sessionProperty` - object, default: `{}`

tests/trino/test-values.yaml:325

  • Add a test scenario for the properties mode (setting type: properties and properties block) to validate that both template branches for sessionProperty are covered.
sessionProperty:

@sudohainguyen
Copy link
Contributor Author

mind reviewing again @nineinchnick ?

@nineinchnick nineinchnick force-pushed the add-session-property branch from 0ccf8af to df31052 Compare July 21, 2025 07:08
@nineinchnick
Copy link
Member

I think plural sessionProperties is a bit better. It configures the session property manager, and session property implies something related to a single property.

@nineinchnick nineinchnick merged commit a9734bf into trinodb:main Jul 21, 2025
11 checks passed
@sudohainguyen sudohainguyen deleted the add-session-property branch July 26, 2025 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed enhancement New feature or request

Development

Successfully merging this pull request may close these issues.

2 participants