prestodb / presto-helm-charts

Presto Helm Charts
https://prestodb.io
Apache License 2.0
13 stars 10 forks source link

Add support for Velox based workers to the Presto helm chart #6

Open mceruzzi-denodo opened 1 month ago

mceruzzi-denodo commented 1 month ago

Description

This pull request adds optional support for using the helm chart with Velox workers. It adds a Velox tree to the values.yaml allowing the user to define if Velox is enabled. When enabled, the worker's image and relavant aspects of configuration are overridden by a customizable Velox based configuration. If Velox is disabled, the helm chart will continue to function as previously. Additionally, this pull request allows the presto configuation properties to be configurable seperately for the coordinator and workers as this can be useful for both Velox and non-Velox scenarios

Motivation and Context

Up until now the helm chart only supports regular Java based Presto. This means that users wanting to utilize Presto with Velox workers would need to find a different option for deployment. This PR seeks to resolve that by enabling Velox based workers to be deployed via the helm chart without impacting any of the existing functionality for those who prefer to use traditional java based workers.

Impact

Additional options in values.yaml

Test Plan

Deployed and ran queries on my AWS based cluster with velox.enabled both true and false checking the settings took affect. Also deployed with edited config.properties and velox.properties ensuring that edits were applied to the cluster.

Contributor checklist

Release Notes

mceruzzi-denodo commented 1 month ago

values.zip Hi Denis,

I went through your comments and made the adjustment. I'm attaching a minimal values.yaml I just used to test the updated files.

To test Prestissimo is in place and working after installing in your enviornment, you can: -Check 'kubectl logs ' to ensure the pod is running with velox. It should be pretty evident from the log messages as you'll see many references to "TaskManager.cpp" -Run some queries against your catalog.

In my case, I have a catalog pointing to my hive metastore. I'm assuming you already have an environment for testing Presto, so you can just switch that catalog out for your own. Although, if you need anything more than what I provided, just let me know.

dnskr commented 4 days ago

Tested with the following commands:

helm install presto charts/presto
helm install presto charts/presto --set prestoCpp.enabled=true
helm install presto charts/presto --set prestoCpp.enabled=true --set mode=ha-cluster
dnskr commented 1 day ago

@mceruzzi-denodo Sorry for the long review process, but I would like to discuss one more question before merging the PR.

Now when we removed velox.properties, WDYT about moving prestoCpp properties to worker property tree? My initial suggestionis to keep them separately was based on the assumption that we will have a lot of PrestoCPP specific configs, but now (after deleting velox.properties) I'm not sure. Of course we can merge the PR as is now and change configuration approach in the future.