-
Notifications
You must be signed in to change notification settings - Fork 180
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
Sync JVM flags with Trino defaults #164
Conversation
As expected, older versions fail to run with:
I could add a condition on the Maybe we should consider removing the default JVM flags from the chart and rely on those in the image, unless explicitly set? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the idea of removing the default config and relying on the docker container defaults. Users can still override it and we then have to just ensure its documented that you take the default from the container you use and update as desired.
70f304a
to
1de61c6
Compare
@@ -32,6 +32,14 @@ data: | |||
-Djdk.nio.maxCachedBufferSize=2000000 | |||
# Allow loading dynamic agent used by JOL | |||
-XX:+EnableDynamicAgentLoading | |||
{{/* only check the version number if the image is not overriden in any way */}} | |||
{{- with .Values.image -}} | |||
{{- if and (eq .repository "trinodb/trino") (not .useRepositoryAsSoleImageReference) (not .registry) (not .digest) (gt (default .Chart.AppVersion .tag | int ) 447 ) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested this on the Helm playground to make sure it works with arbitrary values in all the referenced fields.
1de61c6
to
f6506ad
Compare
I tried preparing another PR that would not override the default |
No description provided.