ChenYi015 commented on code in PR #3196:
URL: https://github.com/apache/celeborn/pull/3196#discussion_r2024491269
##########
charts/celeborn/values.yaml:
##########
@@ -186,10 +186,17 @@ priorityClass:
# -- Specifies the integer value of this priority class, default is
Celeborn master value minus 1000
value: 999999000
-# -- Pod annotations
-podAnnotations: {}
-# key1: value1
-# key2: value2
+master:
+ # -- Pod annotations for Celeborn master pods.
+ annotations:
Review Comment:
> Do we need to document somewhere for each breaking change of the helm
chart? or just treating it as an unstable feature and mentioning the breaking
changes in release notes with some general words?
We have to make some breaking changes to let values like `nodeSelector` can
be configured separately. If you cannot accept breaking changes, I can also
make the Helm chart backward compatible by adding extra templating logic, which
may introduce some complexity.
> seems in other places we put master/worker as a sub-namespace of , for
example:
Currently there are two kinds of values:
1. Work for both master and worker (e.g. `nodeSelector`, `tolerations` and
`securityContext`)
2. Work for master/worker separately (e.g. `priorityClass.{master,worker}`,
`resources.{master,worker}` and `priorityClass.{master,worker}`). Besides,
`masterReplicas` and `workerReplicas` also work separately, but with a
different naming style.
We would better unify the naming sytle of values. I prefer the component
name as a prefix (e.g. `{master,worker}.xxx`) as it is more common and
user-friendly (I think `master.priorityClass.create` is easier to understand
than `priortyClass.master.create`). Actually, we had made these changes already
in our production usage cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
