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

feat: Trino on EKS #437

Merged
merged 38 commits into from
Feb 21, 2024
Merged

feat: Trino on EKS #437

merged 38 commits into from
Feb 21, 2024

Conversation

youngjeong46
Copy link
Contributor

@youngjeong46 youngjeong46 commented Feb 19, 2024

What does this PR do?

🛑 Please open an issue first to discuss any significant work and flesh out details/direction - we would hate for your time to be wasted.
Consult the CONTRIBUTING guide for submitting pull-requests.

Motivation

More

  • Yes, I have tested the PR using my local account setup (Provide any test evidence report under Additional Notes)
  • Mandatory for new blueprints. Yes, I have added a example to support my blueprint PR
  • Mandatory for new blueprints. Yes, I have updated the website/docs or website/blog section for this feature
  • Yes, I ran pre-commit run -a with this PR. Link for installing pre-commit locally

For Moderators

  • E2E Test successfully complete before merge?

Additional Notes

youngjeong46 and others added 30 commits July 21, 2023 10:43
changed image file extensions from .png to .PNG
Changed NodeSelector for worker to be deployed on spot instances
… 4 or above

Using single AZ in Karpenter NodePool to avoid inter-AZ data transfer costs for big data workloads like Trino
Create Glue database, crawler and table in same region where Trino on EKS is deployed
specify region to create Glue database, crawler and table in same region as Trino on EKS deployment
added FT execution example, Final cleanup instructions
Copy link
Contributor

@vara-bonthu vara-bonthu left a comment

Choose a reason for hiding this comment

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

@youngjeong46 Great job on this PR 🚀! I've added a few small suggestions for arranging the code snippets, but other than that, it looks ready to be merged.

@@ -0,0 +1,114 @@
module "eks" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change the name of this file to eks.tf. Then, transfer the code of locals.tf and data.tf into main.tf. Once you've moved everything over, you can go ahead and delete the locals.tf and data.tf files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

distributed-databases/trino/main.tf Outdated Show resolved Hide resolved
@@ -0,0 +1,32 @@
provider "aws" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Move contents from the providers.tf file into main.tf, and after that, you can delete the providers.tf file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

@@ -0,0 +1,156 @@
#---------------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename this file from resources.tf to trino.tf.

Change the name of the resources.tf file to trino.tf.

Then, transfer all Trino-related resources, including the deployment of the Trino add-on module, into this newly renamed trino.tf file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

#---------------------------------------
# Trino Helm Add-on
#---------------------------------------
module "trino_addon" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Transfer this module code to trino.tf to consolidate all Trino-related deployment resources into a single file, making management easier.

Additionally, if you have the capacity, consider adding this as a new add-on under the Data Addons TF module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed - will also add this under the data addons tf module as a follow up.

Comment on lines 13 to 23
variable "namespace" {
description = "Namespace for Trino"
type = string
default = "trino"
}

variable "trino_sa" {
description = "Service Account name for Trino"
type = string
default = "trino-sa"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use local variables or hard code these within trino.tf file and remove from variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

Copy link
Contributor

@vara-bonthu vara-bonthu left a comment

Choose a reason for hiding this comment

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

Thank you 🙌🏼

@vara-bonthu vara-bonthu merged commit 91154ef into awslabs:main Feb 21, 2024
52 of 53 checks passed
@ashwinikumar-sa
Copy link

ashwinikumar-sa commented Feb 21, 2024

Awesome. Long awaited from DoEKS kitchen:) This recipe will help in deploying and scaling cost-efficient Trino clusters in minutes on EKS using Karpenter and Spot instances

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants