Remove query.max-total-memory-per-node from ConfigMap #18
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla. |
|
CLA submitted. |
|
cc: @hovaesco |
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla. |
hashhar
left a comment
hashhar
left a comment
There was a problem hiding this comment.
LGTM % comment.
@erikcw Please squash your commits since it's a single logical change with a message like
Conditionally Remove query.max-total-memory-per-node from ConfigMap
In Trino 370 `query.max-total-memory-per-node` was removed in
https://github.com/trinodb/trino/commit/9ab09e99d6c56fe955697a51a95400c367525133
as a valid configuration and hence must not be added to ConfigMap for
newer Trino versions.
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla. |
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla. |
|
@hashhar I've squashed the commits. I changed the Trino version in your requested commit message. It was actually removed in 369 not 370. |
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla. |
There was a problem hiding this comment.
Just to point out one thing that I don't like. With this we are essentially forcing user of this chart to use images tagged with numbers (with 369 being significant). Even main trinodb/trino dockerhub image has un-bundled tags like 369-arm64 also anyone that adds/modifies this image and retags it with whatever different name will not be able to use this chart.
So in my opinion it is better to have 2 versions of chart, old that works pre 369 and new that works after then make chart work only with forced tag template.
WDYT @hashhar @hovaesco ?
There was a problem hiding this comment.
That is a very good point that I had not considered.
Would a simpler solution be to render this config only if the value is provided?
i.e. change default value to null or empty value. Render only non-empty values in ConfigMap. Add a description for the config that explains to leave it unset when using Trino >= 369?
There was a problem hiding this comment.
Having two separate versions of the chart would be harder to maintain, manage etc.
@hashhar approach sounds good. I would also comment maxTotalMemoryPerNode by default with the suggested comment from above.
There was a problem hiding this comment.
Sounds good @hashhar
With that change user would only need to remove maxTotalMemoryPerNode form they deployment files to be able to use versions >369.
There was a problem hiding this comment.
The problem with leaving it in for the sake of backwards compatibility is that users are going to update to the latest chart and potentially have a broken deployment. In my experience with other projects, this usually results in a bunch of issues/support requests being needlessly generated.
In my eyes, it makes the most sense to remove it entirely and bump the chart version to 0.4.0 as a breaking change. Users who need to stay on Trino <369 should continue to use the 0.3 chart. This should result in very little/no ongoing maintenance.
Yes, this will likely also result in a few broken deployments when users upgrade their helm chart to >=0.4 and pin their Trino tag to an old version. Seems favorable as opposed to adding the additional complexity to the chart to and future maintenance.
There was a problem hiding this comment.
I'm not very familiar with how people interact with Helm. If chart updates aren't applied automatically and if Helm can catch that there are some unused values provided in deployment (i.e. keys that aren't part of values.yml) then what @erikcw is suggesting is also fine.
I don't think there are going to be any huge changes in chart so people can continue using older chart until they reach Trino 369?
@electrum any opinion?
|
Thanks for identifying the root cause and working on the fix. I agree with @erikcw , that it would be best to bump the Helm Chart version (and appVersion) simultaneously to signal that there are breaking changes and preserve status quo in 0.3.0, while allowing everyone else to adopt new/latest versions of Trino with an updated Helm Chart. |
Yes. The helm should not do this if logic hack based on the image tag. It should be handled via the chart version and in the chart it can specify the application version as well. |
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla. |
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla. |
|
Ok, since it seems a consensus is forming around cutting a new version and removing the conditional statements, I've gone ahead and made those edits to this PR. |
hashhar
left a comment
hashhar
left a comment
There was a problem hiding this comment.
This is the best we can do IMO.
Even though Helm does provide ways to validate things it's painful to do. We can look at adding either a JSON Schema (as documented at https://helm.sh/docs/topics/charts/#schema-files) or something like https://austindewey.com/2018/12/28/helm-tricks-input-validation-with-required-and-fail/ for deeper validation going into future (e.g. ensuring memory per node + headroom < jvm memory etc.)
|
cc: @martint for CLA. this is ready to merge otherwise. |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
Thanks for contributing @erikcw and sorry about the long back and forth. I'll send an announcement on the Trino Slack so that chart users are aware of this change. |
6 participants
query.max-total-memory-per-node was removed from trino 369.
This PR updates the chart accordingly. Since this chart defaults to running the
latesttrino release, its presence prevents trino from booting.