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

Added dockerization for the Trino Gateway #113

Merged
merged 1 commit into from
Jan 26, 2024

Conversation

rdsarvar
Copy link
Contributor

Adding documentation, image creation, and compose file modifications required for running the Trino Gateway in a docker container.

Docker setup based on what @wendigo did with Trino

Potential implementation for #86

Copy link

cla-bot bot commented Dec 17, 2023

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 [email protected]. 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

@mosabua
Copy link
Member

mosabua commented Dec 20, 2023

So I have not really looked at the details of this PR at all, but here are a few points to consider

  • we should use same maven and docker setup as in trino itself so that the docker build is part of the maven build
  • we should use the same container image as base
  • we should use Eclipse Temurin 17
  • we will try to get to Java 21 and Eclipse Temurin 21 over time
  • release script for publishing should be in this repo (I think I saw it in the PR already) and should support multi arch
  • we aim to use the container image as base for a helm chart for Trino Gateway

I am currently going through a refactor PR and then we will try to get all other PRs reviewed and merged .. including this one. Thank you for all the work you have done already.

Also would love some feedback from their practical experience with container images for Trino Gateway from @willmostly @andythsu @vishalya @Chaho12 and others.

@Chaho12 Chaho12 mentioned this pull request Dec 22, 2023
Copy link

cla-bot bot commented Dec 29, 2023

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 [email protected]. 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

@mosabua
Copy link
Member

mosabua commented Jan 5, 2024

Please rebase and resolve conflicts.

@rdsarvar rdsarvar force-pushed the add_docker_for_gateway branch from 405a06e to 50a3254 Compare January 6, 2024 02:11
Copy link

cla-bot bot commented Jan 6, 2024

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 [email protected]. 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

@mosabua
Copy link
Member

mosabua commented Jan 9, 2024

@cla-bot check

@cla-bot cla-bot bot added the cla-signed label Jan 9, 2024
Copy link

cla-bot bot commented Jan 9, 2024

The cla-bot has been summoned, and re-checked this pull request!

@mosabua
Copy link
Member

mosabua commented Jan 9, 2024

Let me know if you are ready for review again @rdsarvar

@rdsarvar rdsarvar force-pushed the add_docker_for_gateway branch 2 times, most recently from ee6e0f2 to 9789280 Compare January 22, 2024 01:55
@rdsarvar rdsarvar changed the title [WIP] Added dockerization for the Trino Gateway Added dockerization for the Trino Gateway Jan 22, 2024
@rdsarvar rdsarvar marked this pull request as ready for review January 22, 2024 01:55
@mosabua
Copy link
Member

mosabua commented Jan 25, 2024

I think we should merge this setup to get started and improve from there. What do you think @willmostly @Chaho12 @vishalya @wendigo

We just cut version 5 so we have some time to improve before we ship this .. and we could even still release 6 without pushing the container.

@mosabua
Copy link
Member

mosabua commented Jan 25, 2024

Please address any feedback that it still outstanding and squash commits @rdsarvar

Adding documentation, image creation, and compose file modifications
required for running the Trino Gateway in a docker container.
@rdsarvar rdsarvar force-pushed the add_docker_for_gateway branch from 9789280 to 1c08e1b Compare January 26, 2024 01:39
Copy link
Member

@Chaho12 Chaho12 left a comment

Choose a reason for hiding this comment

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

Sounds good :)
Btw do you need gateway-ha-config-docker.yml file? I find it duplicate with gateway-ha-config.yml file. Will there be any needs for config to be different?

@rdsarvar
Copy link
Contributor Author

rdsarvar commented Jan 26, 2024

Sounds good :) Btw do you need gateway-ha-config-docker.yml file? I find it duplicate with gateway-ha-config.yml file. Will there be any needs for config to be different?

The only real reason I did it is to be able to target the postgres container from inside of the gateway container:

  jdbcUrl: jdbc:postgresql://postgres:5432/trino_gateway_db

Due to 'localhost' inside of the container referring to itself and not the underlying host running the container.

If we don't want to use the isolated bridged networking mode, we could use host network mode and that would bind the containers directly to the network running the daemon. I'm just not 100% sure how well that'd play if we eventually wanted to run this in CI, would need to see if using docker-in-docker if it thinks of the DinD host as the network to use, or if it tries to get to the physical host. 🤔

Do you think there's an easy way to maybe at runtime (docker compose up gateway) convert instances of 127.0.0.1/localhost in the mounted file to host.docker.internal?

@mosabua
Copy link
Member

mosabua commented Jan 26, 2024

I think its fine to use a separate config file and default to PostgreSQL... something I want to do anyway.

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.

Okay .. I tested it all out now and got it all working.

The PR as it stands is functional and good enough to merge.

There were a bunch of things I ran into with regards to macos/arm64 and maven wrapper. I will send a follow up PR for improvements related to that.

@mosabua mosabua merged commit bd4b16d into trinodb:main Jan 26, 2024
2 checks passed
@github-actions github-actions bot added this to the 6 milestone Jan 26, 2024
@rdsarvar rdsarvar deleted the add_docker_for_gateway branch January 27, 2024 14:02
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.

4 participants