Skip to content
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 enabling dynamic catalogs in containers #19621

Merged
merged 1 commit into from
Mar 1, 2024

Conversation

nineinchnick
Copy link
Member

Description

Allow enabling dynamic catalogs in containers without mounting the whole config file.

We get lots of questions in Slack on how to run Trino in a container, and having to mount config files as volumes makes it much harder. This change allows to only have to specify the CATALOG_MANAGEMENT environmental variable:

docker run -it --rm --name trino -e CATALOG_MANAGEMENT=dynamic trinodb/trino:latest

Most of the current limitations of dynamic catalogs don't apply when testing Trino locally in a container. In production deployments, the default config file needs to be overwritten anyway, so this should not add any risk of enabling dynamic catalogs accidentally.

If #19612 gets merged, I'll update docs/src/main/sphinx/installation/containers.md with the above.

Additional context and related issues

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Nov 3, 2023
@mosabua
Copy link
Member

mosabua commented Nov 6, 2023

#19612 is now merged so you can rebase and proceed here

@nineinchnick
Copy link
Member Author

@hashhar you don't think this is a good idea?

@mosabua
Copy link
Member

mosabua commented Nov 7, 2023

I am torn about this approach. What makes this config.properities setting different from all others and more valuable and therefore worth exposing in this simple manner.

Could we have a more generic approach that just points to a file that includes properties settings that are appended to the built in setup?

Or could we document how to set it up with just mounting a local folder and use this (and maybe routine storage in a memory connector catalog as example?).

@nineinchnick
Copy link
Member Author

I admit this creates a precedence. But making this single property configurable via env vars allow running Trino and using any connector available by default without having to mount any other files.

just mounting a local folder

This is the hardest part of running a container. A lot of people can't figure out where to save files, how relative paths work, etc. Even before getting into permissions.

@mosabua
Copy link
Member

mosabua commented Nov 7, 2023

Fair enough @nineinchnick .. I think we are okay with merging this but should make sure we dont go crazy with adding more. Wdyt @hashhar ?

@hashhar
Copy link
Member

hashhar commented Nov 8, 2023

The setting a precedence is basically what I'm concerned about. There are even more useful configs which can get this treatment (e.g. all the query-manager configs).
Also this doesn't fix the "out-of-box" issues people can have when using dynamic catalogs (e.g. filesystem permissions).

IMO the solution should be improved documentation instead of applying magic.

@mosabua
Copy link
Member

mosabua commented Nov 8, 2023

Sounds good @hashhar .. I tend to agree. How about we add info on how to add stuff to the config.properties .. and use this as an example. Probably have to detail how to look at the existing config.properties file in the container and then copy it out and modify.

Are you good to run with this in the current PR @nineinchnick ?

@nineinchnick
Copy link
Member Author

That changes the original idea of this pr so we should start from scratch

@mosabua
Copy link
Member

mosabua commented Nov 8, 2023

Ok .. just include those fixes on the docs please ;-)

@wendigo wendigo reopened this Feb 28, 2024
@wendigo
Copy link
Contributor

wendigo commented Feb 28, 2024

Let's revisit this.

@nineinchnick
Copy link
Member Author

I don't have any other ideas how we could easily enable dynamic catalogs in the container. I don't want to keep a stale PR open, I'll close this again in a week if there's no discussion.

@wendigo
Copy link
Contributor

wendigo commented Feb 28, 2024

I like the approach proposed here. @mosabua what's your concern?

Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

A couple of tweaks.. but then lets ship this

docs/src/main/sphinx/installation/containers.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/installation/containers.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/installation/containers.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/installation/containers.md Outdated Show resolved Hide resolved
@mosabua mosabua force-pushed the container-dyn-cats branch from 53efdb8 to e9fc020 Compare March 1, 2024 18:21
- Without mounting the whole config file
- Fix typo in catalog management properties docs
@mosabua mosabua force-pushed the container-dyn-cats branch from e9fc020 to 1c42958 Compare March 1, 2024 18:24
@mosabua mosabua merged commit ad30a06 into trinodb:master Mar 1, 2024
3 checks passed
@github-actions github-actions bot added this to the 440 milestone Mar 1, 2024
@nineinchnick nineinchnick deleted the container-dyn-cats branch May 27, 2024 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants