#103 thrift server chart #121
Conversation
| @@ -18,14 +18,6 @@ | |||
| <plugin> | |||
| <groupId>${group.helm.plugin}</groupId> | |||
| <artifactId>helm-maven-plugin</artifactId> | |||
| <configuration> | |||
There was a problem hiding this comment.
I: Unused relic from a previous copy-paste
| @@ -2,7 +2,7 @@ | |||
| apiVersion: v1 | |||
| kind: ConfigMap | |||
| metadata: | |||
| name: {{ .Release.Name }}-spark-conf | |||
| name: {{ .Release.Name }}-shs-spark-conf | |||
There was a problem hiding this comment.
I: There are several changes like this-- In a downstream project, merely using .Release.Name resulted in both subcharts generating their k8s resources with the same names, causing conflicts.
| sparkConf: | | ||
| spark.hadoop.fs.s3a.endpoint=http://s3-local:4566 | ||
| spark.hadoop.fs.s3a.access.key=#[[${env:AWS_ACCESS_KEY_ID}]]# |
There was a problem hiding this comment.
I: The #[[ ... ]]# syntax instructs Velocity not to attempt to parse the inner text.
| eventVolume: |
There was a problem hiding this comment.
I: Fixed inconsistent indentation
| @@ -2,7 +2,7 @@ | |||
| apiVersion: apps/v1 | |||
| kind: Deployment | |||
| metadata: | |||
| name: {{ .Release.Name }} | |||
| name: {{ .Release.Name }}-shs | |||
There was a problem hiding this comment.
Q: Wouldn't the release name already mention that it's a spark history server? So it'd be something like my-spark-history-shs which feels odd.
There was a problem hiding this comment.
The problem here is how it works when unified in a single chart in a downstream project. When you generate out spark-infrastructure downstream, with a dependency on both the shs chart and sts chart, when the chart is deployed, .Release.Name is identical for both (specifically, spark-infrastructure)
| | deployment.replicas | Number of replicas for the Spark Thrift Server Deployment. | 1 | | ||
| | deployment.image.repository | Repository for the Spark Thrift Server image. | "apache/spark" | | ||
| | deployment.image.tag | Tag for the Spark Thrift Server image. | "3.5.1" | | ||
| | deployment.image.imagePullPolicy | Image pull policy for the Spark Thrift Server image. | "IfNotPresent" | |
There was a problem hiding this comment.
A: Why not just deployment.image.pullPolicy or deployment.imagePullPolicy?
There was a problem hiding this comment.
This is consistent with most of our other aissemble charts-- The only real departure from our normal default is storing it under the deployment tag as opposed to having a standalone image section.
| - mountPath: /tmp/jars | ||
| name: sts-jars | ||
| {{- end }} | ||
| {{- if not (empty .Values.dependencies.jars) }} |
There was a problem hiding this comment.
Q/A: Can we not just do this with one init container with chained commands?
There was a problem hiding this comment.
We can, but I'm not sure that it makes much difference either way. I do think that in the event of an issue, it's easier to debug this way as opposed to one big bash one-liner.
| {{- toYaml .Values.deployment.volumeMounts }} | ||
| {{- end }} | ||
| volumes: | ||
| {{- if or (not (empty .Values.dependencies.packages)) (not (empty .Values.dependencies.jars)) }} |
There was a problem hiding this comment.
A: See this in a few places in this template. Maybe worth pulling out into a common place
There was a problem hiding this comment.
I was having issues having it work well from within _helpers.tpl. I'd like to take another shot at getting it to work, but don't know that it's worth holding up this ticket for.
| service: | ||
| name: {{ .backend.service.name | default $releasename }} | ||
| port: | ||
| number: {{ .backend.service.port.number }} |
There was a problem hiding this comment.
S: We know the default service name and port. We should just use the values already set on Values.service. If someone actually tried to change this to a different value, I'd argue that'd be inherently incorrect. I could see allowing them to extend with extra services, but not to replace the actual service definition which we're creating in this chart.
There was a problem hiding this comment.
Could have a static path definition for the service provided by this chart, then allow them to add extra with a .otherPaths config.
There was a problem hiding this comment.
On further examination, I would actually be inclined to take this whole section out and replace it with:
{{ .paths | toYaml | nindent 10 }}
or something to that effect
peter-mcclonski
force-pushed
the
103-thrift-server-chart
branch
from
e709c29
to
03dc3f5
Compare
Signed-off-by: Peter McClonski <[email protected]>
peter-mcclonski
force-pushed
the
103-thrift-server-chart
branch
from
03dc3f5
to
17668fb
Compare
No description provided.