-
Notifications
You must be signed in to change notification settings - Fork 919
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
[K8S][HELM] Add Hadoop configuration support #6046
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6046 +/- ##
============================================
- Coverage 61.03% 60.99% -0.05%
Complexity 23 23
============================================
Files 623 623
Lines 37162 37162
Branches 5035 5035
============================================
- Hits 22682 22666 -16
- Misses 12029 12033 +4
- Partials 2451 2463 +12 ☔ View full report in Codecov by Sentry. |
{{- tpl . $ | nindent 4 }} | ||
{{- end }} | ||
{{- with .Values.hadoopConf.hdfsSite }} | ||
hdfs-site.xml: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it's likely to put additional configuration files under HADOOP_CONF_DIR in practice, for example, hive-site.xml
, hbase-site.xml
, so we may want to allow the user to inject custom files.
Another concern is that embeding large-XML into YAML may be not user-friendly, it's easy to mess up the indent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely agree. The approach I often see is providing both options: either provide a (templated) inline config - which as you said can get unwieldy fast - or provide the name of an existing config map. The latter allows creating that config map from a file or some other way, without relying on functionality in the chart.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chgl are there examples of some projects that have similar cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ones that I am aware of:
- https://github.com/bitnami/charts/blob/main/bitnami/zookeeper/values.yaml#L203-L214, - https://github.com/bitnami/charts/blob/main/bitnami/postgresql/values.yaml#L293-L309.
- Superset: https://github.com/apache/superset/blob/master/helm/superset/values.yaml#L135 (fairly extreme since entire python snippets are inlined) with https://github.com/apache/superset/blob/master/helm/superset/values.yaml#L50 as an option to reference an existing secret.
- akhq: https://github.com/tchiotludo/akhq/blob/dev/helm/akhq/values.yaml#L36-L46
🔍 Description
Issue References 🔗
The PR adds basic support for Hadoop configuration files to be used by Apache Kyuubi, Apache Spark etc.
Describe Your Solution 🔧
The PR adds:
ConfigMap
that has to be mounted to Kyuubi pods as files.HADOOP_CONF_DIR
environment variable to Kyuubi pods.Notes:
ConfigMap
change restarts Kyuubi pods!core-site.xml
andhdfs-site.xml
files are added. Not sureyarn-site.xml
,mapred-site.xml
or other files should be added. Please, let me know if so.Types of changes 🔖
Test Plan 🧪
Install the chart with configured Spark and the following values:
Run
beeline
from Kyuubi pod:./bin/beeline -u 'jdbc:hive2://kyuubi-thrift-binary:10009' -n test-user
Check specified property values:
Checklist 📝
Be nice. Be informative.