docs: clarify server.workerExtraConfig / coordinatorExtraConfig#421
docs: clarify server.workerExtraConfig / coordinatorExtraConfig#421ttagu99 wants to merge 1 commit into
Conversation
`server.workerExtraConfig` and `server.coordinatorExtraConfig` currently show up in README as bare one-liners with no description. In practice these are the only supported way to set role-specific `config.properties` entries — `additionalConfigProperties` merges into *both* coordinator and worker, which makes Trino fail with `Configuration property 'X' was not used` on whichever role does not recognise the property (common with `http-server.authentication.type` and related JWT/OAuth2 settings). Add helm-docs descriptions and a JWT example to save users a crash-then-diagnose round trip. No template/runtime changes.
|
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 cla@trino.io. 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 |
|
@cla-bot check |
|
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 cla@trino.io. 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 |
|
The cla-bot has been summoned, and re-checked this pull request! |
nineinchnick
left a comment
nineinchnick
left a comment
There was a problem hiding this comment.
Thank you. This is a valuable change.
| # `workerExtraConfig` or `coordinatorExtraConfig` instead — otherwise Trino's | ||
| # strict `Configuration property 'X' was not used` check crashes the pod that | ||
| # does not recognise the property. |
There was a problem hiding this comment.
I understand you were frustrated and want to make these docs extra clear, but this last part repeats what's stated in the first sentence, so I'd remove it.
| # `workerExtraConfig` or `coordinatorExtraConfig` instead — otherwise Trino's | |
| # strict `Configuration property 'X' was not used` check crashes the pod that | |
| # does not recognise the property. | |
| # `workerExtraConfig` or `coordinatorExtraConfig` instead. |
| # PASSWORD) that workers do not accept — adding them via | ||
| # `additionalConfigProperties` crashes workers with | ||
| # `Configuration property 'http-server.authentication.type' was not used`. |
There was a problem hiding this comment.
You might clarify this in the docs for additionalConfigProperties that the properties added there will be used in all node types and should not include node-specific settings. additionalConfigProperties is currently not documented at all.
| # PASSWORD) that workers do not accept — adding them via | |
| # `additionalConfigProperties` crashes workers with | |
| # `Configuration property 'http-server.authentication.type' was not used`. | |
| # PASSWORD) that workers do not accept. |
2 participants
What
Add helm-docs descriptions for
server.workerExtraConfigandserver.coordinatorExtraConfig. They are currently listed in the README with only the bare property name and no guidance, which left me chasing a 30-minute coordinator/worker mismatch the first time I tried to enable JWT authentication.Why
additionalConfigPropertiesis merged into bothconfig.propertiesfiles (coordinator + worker). That is fine for cluster-wide properties, but role-specific properties — the authentication flags being the most common example — cause whichever pod does not recognise the property to crash with:The chart already provides the correct escape hatch (
server.coordinatorExtraConfig/server.workerExtraConfig) and templates/configmap-coordinator.yaml / templates/configmap-worker.yaml already wire them through. The only thing missing is the documentation pointing users to them before they tryadditionalConfigPropertiesand hit the crash.What changed
charts/trino/values.yaml— add# server.workerExtraConfig --and# server.coordinatorExtraConfig --helm-docs comments with a short explanation plus a copy-pasteable example. Added a JWT authentication example oncoordinatorExtraConfigsince that is the typical production case.charts/trino/README.md— manually regenerate just those two entries to keep the diff minimal. (My localhelm-docs v1.14.2 --chart-search-root=charts --document-dependency-valuesregenerates the rest of the file with whitespace-only differences, so I reverted those lines.)Not changed
Test plan
helm-docs v1.14.2 --chart-search-root=chartsruns clean.additionalConfigPropertiesworker crashes.