-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Allow referencing env.sh vars in configs #6635
Allow referencing env.sh vars in configs #6635
Conversation
@nineinchnick what about adding the Or, maybe even better, we could ask the env.sh script to define an array variable (or comma-separated) with names of all env vars it intends to export. wdyt? |
7dafb43
to
1d3673d
Compare
It looks cool! Turns out I can |
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.
Looks great!
"cat > /etc/trino/catalog/hive.properties <<\"EOT\"\n" + | ||
"connector.name=hive-hadoop2\n" + | ||
"hive.metastore.uri=thrift://localhost:9083\n" + | ||
"hive.metastore.uri=thrift://localhost:${ENV:HMS_PORT}\n" + |
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.
looks like local testing modification
it would be nice to have a test for that, but let's make it a dedicated one
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.
It's not a local testing modification. It extends the test coverage to ensure envs are passed properly. This is an integration test, not a unit test. I'm concerned that a dedicated test would increase the total duration. Are you sure it's worth putting in a separate test?
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.
How much time we would add and how much time the job already takes today?
BTW the test is not very directly (there is no explicit assertion, so it could in theory become ignored by the server.)
Can we inject eg node.environment
and verify what's the actual value of that?
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.
Trino throws an error when configuration files reference an unset env var.
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.
These two tests in ServerIT run in the pipeline in slightly over a minute:
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 70.948 s - in io.trino.server.rpm.ServerIT
so I'd expect a separate test to add ~30 seconds.
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.
And the job total time is 20 minute. I think it's reasonable to have this as a separate test, but i think i am OK keeping this inline.
would it be possible to add an explicit assertion for the feature?
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 added a custom node id and an assert for it.
1d3673d
to
49eb436
Compare
"cat > /etc/trino/catalog/hive.properties <<\"EOT\"\n" + | ||
"connector.name=hive-hadoop2\n" + | ||
"hive.metastore.uri=thrift://localhost:9083\n" + | ||
"hive.metastore.uri=thrift://localhost:${ENV:HMS_PORT}\n" + |
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.
How much time we would add and how much time the job already takes today?
BTW the test is not very directly (there is no explicit assertion, so it could in theory become ignored by the server.)
Can we inject eg node.environment
and verify what's the actual value of that?
58081b5
to
c125c60
Compare
c125c60
to
9ba11df
Compare
core/trino-server-rpm/src/test/java/io/trino/server/rpm/ServerIT.java
Outdated
Show resolved
Hide resolved
9ba11df
to
63ae0a7
Compare
63ae0a7
to
224bcf3
Compare
Allow referencing env vars from
env.sh
in configs.Since this would allow users to add their custom vars to
env.sh
treat it as a config file so it won't be removed automatically on uninstall. Note that by defaultnode.properties
also won't be removed, as reported in #314