Add support for Velox based workers to the Presto helm chart

[mceruzzi-denodo]

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

• Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
• PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
• Documented new properties (with its default value), SQL syntax, functions, or other functionality.
• If release notes are required, they follow the release notes guidelines.
• Adequate tests were added if applicable.
• CI passed.

Release Notes

[dnskr]

@mceruzzi-denodo Thank you for the PR!

Please, have a look at minor comments I left.
Also, I would highly appreciate it if you could share minimal values.yaml and scenario to test Velox setup.

[mceruzzi-denodo]

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 <worker_pod>' 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]

Looks like presto.version should be added to configmap-resource-manager.yaml, otherwise ha-cluster mode doesn't work properly. Could you please check it?

[mceruzzi-denodo]

Yes, confirmed that presto.version is needed for the discovery server to properly detect the Resource Manager. I've Added this.

Also, I noticed we are calling the new version as Presto C++. should we keep the section in the charts as "velox" or should we rename it as "prestocpp" or similar?

[dnskr]

@tdcmeehan Do you have any strong opinion on how we should name it (velox, prestocpp, native etc)? Otherwise let's keep it velox for now.

[tdcmeehan]

prestocpp is the naming we're using and what we should adopt here.

[dnskr]

Looks good to me. Thank you for your effort @mceruzzi-denodo !

@tdcmeehan Could you please have a look if configs and approach look ok?

[tdcmeehan]

Just some nits

[tdcmeehan]

Do we need this?

[mceruzzi-denodo]

removed

[tdcmeehan]

We've deprecated velox.properties, so let's just remove this: prestodb/presto#21962

[mceruzzi-denodo]

sure, removed.

[tdcmeehan]

LGTM. Let's squash commits.

[dnskr]

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]

@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.