[#1028] improvement(operator): Make rss-env.sh and HadoopConfig configurable via ConfigMap #1193
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1193 +/- ##
============================================
- Coverage 53.64% 44.23% -9.42%
============================================
Files 391 20 -371
Lines 22411 2360 -20051
Branches 1875 0 -1875
============================================
- Hits 12023 1044 -10979
+ Misses 9682 1245 -8437
+ Partials 706 71 -635 see 371 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -154,6 +154,8 @@ function load_rss_env { | |||
| set +o nounset | |||
| if [ -f "${RSS_CONF_DIR}/rss-env.sh" ]; then | |||
| RSS_ENV_SH="${RSS_CONF_DIR}/rss-env.sh" | |||
| elif [ -f "${RSS_HOME}/conf/rss-env.sh" ]; then | |||
| RSS_ENV_SH="${RSS_HOME}/conf/rss-env.sh" | |||
There was a problem hiding this comment.
why is this needed?
RSS_CONF_DIR should already default to ${RSS_HOME}/conf.
| @@ -28,6 +28,7 @@ data: | |||
| rss.coordinator.server.heartbeat.timeout 30000 | |||
| rss.jetty.http.port 19996 | |||
| rss.rpc.server.port 19997 | |||
| # rss.security.hadoop.kerberos.keytab.file /data/rssadmin/hadoop/conf/uniffle.keytab | |||
There was a problem hiding this comment.
I don't think this is related?
|
|
||
| # HADOOP_CONF_DIR, Hadoop configuration directory (Default: ${HADOOP_HOME}/etc/hadoop) | ||
| HADOOP_CONF_DIR="/data/rssadmin/hadoop/conf" |
There was a problem hiding this comment.
why so we need to set HADOOP_CONF_DIR? I believe ${HADOOP}/etc/hadoop is a good default place
| namespace: kube-system | ||
| data: | ||
| hadoop-env.sh: |- | ||
| export JAVA_HOME=/opt/java/openjdk |
There was a problem hiding this comment.
I don't think we should set JAVA_HOME here. It should inherit from containers' pre-set JAVA_HOME.
| hadoop-env.sh: |- | ||
| export JAVA_HOME=/opt/java/openjdk | ||
| export HADOOP_HOME=/data/rssadmin/hadoop | ||
| export HADOOP_CONF_DIR=/data/rssadmin/hadoop/conf |
There was a problem hiding this comment.
ditto. Why do we need to set this?
| # log4j.properties | ||
| krb5.conf: |- | ||
| [libdefaults] | ||
| default_realm = EXAMPLE.COM |
There was a problem hiding this comment.
Please add some comments to indicate these values should be replaced.
| volumes: | ||
| - name: hadoop-config | ||
| configMap: | ||
| name: hadoop-configuration |
There was a problem hiding this comment.
I don't think we need to update the design.md to reflect the change.
| ## Setup or Update Uniffle Configurations | ||
|
|
||
| We can refer to [rss configuration](../../deploy/kubernetes/operator/examples/configuration.yaml) | ||
|
|
||
| ``` | ||
| kubectl apply -f ${configuration-yaml-file} | ||
| ``` | ||
|
|
||
| ## Setup or Update Hadoop Configurations | ||
|
|
||
| We can refer to [hadoop configuration](../../deploy/kubernetes/operator/examples/hadoop-configuration.yaml) | ||
|
|
||
| ``` | ||
| kubectl apply -f ${hadoop-configuration-yaml-file} | ||
| ``` | ||
|
|
There was a problem hiding this comment.
this should goes to examples.md.
| rss.storage.type MEMORY_LOCALFILE | ||
| # rss.security.hadoop.kerberos.keytab.file /data/rssadmin/hadoop/conf/uniffle.keytab |
|
@qijiale76 Could you finish this PR? |
What changes were proposed in this pull request?
Make
rss-env.shandhadoopconfigconfigurable via ConfigMap.Add
dynamic_client.confin configuration example file.Add
hadoop-configuration.yamlexample file.Improve docs.
Why are the changes needed?
Fix: #1028
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Tested in our cluster.