You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "mrproliu (via GitHub)" <gi...@apache.org> on 2025/11/07 11:59:12 UTC
[PR] Support data node list in the liaison [skywalking-banyandb-helm]
mrproliu opened a new pull request, #39:
URL: https://github.com/apache/skywalking-banyandb-helm/pull/39
Resolve https://github.com/apache/skywalking/issues/13510, adding a new configuration `dataNodeListEnv` for add a new `BYDB_DATA_NODE_LIST` env to the liaison node.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support data node list in the liaison [skywalking-banyandb-helm]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #39:
URL: https://github.com/apache/skywalking-banyandb-helm/pull/39#discussion_r2504368080
##########
chart/templates/_helpers.tpl:
##########
@@ -98,4 +98,32 @@ EtcdEndpoints
{{- end }}
- name: BYDB_ETCD_ENDPOINTS
value: "{{- $endpoints | join "," -}}"
-{{- end }}
\ No newline at end of file
+{{- end }}
+
+{{/*
+Generate data node names list based on passing roles
+*/}}
+{{- define "banyandb.dataNodeList" -}}
+{{- $dataNodes := list }}
+{{- $context := index . 0 }}
+{{- $configuredRoles := index . 1 }}
+{{- $fullname := include "banyandb.fullname" $context }}
+{{- range $roleName, $roleConfig := $context.Values.cluster.data.roles }}
+ {{- $shouldInclude := false }}
+ {{- if eq (len $configuredRoles) 0 }}
+ {{- $shouldInclude = true }}
+ {{- else }}
+ {{- if has $roleName $configuredRoles }}
+ {{- $shouldInclude = true }}
+ {{- end }}
+ {{- end }}
+ {{- if $shouldInclude }}
+ {{- $replicas := $roleConfig.replicas | default $context.Values.cluster.data.nodeTemplate.replicas }}
+ {{- range $i := until (int $replicas) }}
+ {{- $nodeName := printf "%s-data-%s-%d" $fullname $roleName $i }}
+ {{- $dataNodes = append $dataNodes $nodeName }}
+ {{- end }}
+ {{- end }}
+{{- end }}
+{{- $dataNodes | join "," -}}
+{{- end }}
Review Comment:
[nitpick] The nested if statements for `$shouldInclude` can be simplified using a single conditional expression, improving readability. Consider: `{{- $shouldInclude := or (eq (len $configuredRoles) 0) (has $roleName $configuredRoles) }}`
##########
chart/templates/_helpers.tpl:
##########
@@ -98,4 +98,32 @@ EtcdEndpoints
{{- end }}
- name: BYDB_ETCD_ENDPOINTS
value: "{{- $endpoints | join "," -}}"
-{{- end }}
\ No newline at end of file
+{{- end }}
+
+{{/*
+Generate data node names list based on passing roles
Review Comment:
Corrected grammar: 'based on passing roles' should be 'based on passed roles' or 'based on specified roles'.
```suggestion
Generate data node names list based on specified roles
```
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support data node list in the liaison [skywalking-banyandb-helm]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #39:
URL: https://github.com/apache/skywalking-banyandb-helm/pull/39#discussion_r2508486175
##########
chart/templates/_helpers.tpl:
##########
@@ -98,4 +98,27 @@ EtcdEndpoints
{{- end }}
- name: BYDB_ETCD_ENDPOINTS
value: "{{- $endpoints | join "," -}}"
-{{- end }}
\ No newline at end of file
+{{- end }}
+
+{{- define "banyandb.hasDataNodeListValue" -}}
+{{- $dataNodeList := include "banyandb.dataNodeListValue" . }}
+{{- if ne $dataNodeList "" }}true{{- end }}
Review Comment:
The helper function checks if the data node list is non-empty but doesn't verify if the feature is enabled via `cluster.liaison.dataNodeListEnv.enabled`. This means the environment variable will be set even when `enabled: false` in values.yaml, as long as there are data nodes with the "hot" role. The condition should also check `.Values.cluster.liaison.dataNodeListEnv.enabled` before returning true.
```suggestion
{{- if and .Values.cluster.liaison.dataNodeListEnv.enabled (ne $dataNodeList "") }}true{{- end }}
```
##########
chart/templates/_helpers.tpl:
##########
@@ -98,4 +98,27 @@ EtcdEndpoints
{{- end }}
- name: BYDB_ETCD_ENDPOINTS
value: "{{- $endpoints | join "," -}}"
-{{- end }}
\ No newline at end of file
+{{- end }}
+
+{{- define "banyandb.hasDataNodeListValue" -}}
+{{- $dataNodeList := include "banyandb.dataNodeListValue" . }}
+{{- if ne $dataNodeList "" }}true{{- end }}
+{{- end }}
+
+{{/*
+Generate data node names list for "hot" role only
+*/}}
+{{- define "banyandb.dataNodeListValue" -}}
+{{- $dataNodes := list }}
+{{- $fullname := include "banyandb.fullname" . }}
+{{- range $roleName, $roleConfig := .Values.cluster.data.roles }}
+ {{- if eq $roleName "hot" }}
+ {{- $replicas := $roleConfig.replicas | default $.Values.cluster.data.nodeTemplate.replicas }}
+ {{- range $i := until (int $replicas) }}
+ {{- $nodeName := printf "%s-data-%s-%d" $fullname $roleName $i }}
+ {{- $dataNodes = append $dataNodes $nodeName }}
+ {{- end }}
+ {{- end }}
+{{- end }}
+{{- $dataNodes | join "," -}}
+{{- end }}
Review Comment:
The implementation hardcodes filtering for the "hot" role only (line 115), but the configuration in values.yaml provides a `cluster.liaison.dataNodeListEnv.roles` field to specify which roles to include. The implementation should respect the configured roles list. When `roles` is empty, it should include all roles; otherwise, it should only include the specified roles. The current implementation ignores this configuration entirely.
##########
chart/templates/_helpers.tpl:
##########
@@ -98,4 +98,27 @@ EtcdEndpoints
{{- end }}
- name: BYDB_ETCD_ENDPOINTS
value: "{{- $endpoints | join "," -}}"
-{{- end }}
\ No newline at end of file
+{{- end }}
+
+{{- define "banyandb.hasDataNodeListValue" -}}
+{{- $dataNodeList := include "banyandb.dataNodeListValue" . }}
+{{- if ne $dataNodeList "" }}true{{- end }}
+{{- end }}
+
+{{/*
+Generate data node names list for "hot" role only
Review Comment:
The comment states this generates data node names for "hot" role only, which is inconsistent with the values.yaml documentation that suggests the `roles` field should control which roles are included. The comment should be updated to reflect the intended behavior where configurable roles determine which data nodes are included.
```suggestion
Generate data node names list for all configured roles as specified in the `roles` field
```
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support data node list in the liaison [skywalking-banyandb-helm]
Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily commented on code in PR #39:
URL: https://github.com/apache/skywalking-banyandb-helm/pull/39#discussion_r2505939130
##########
chart/templates/_helpers.tpl:
##########
@@ -98,4 +98,32 @@ EtcdEndpoints
{{- end }}
- name: BYDB_ETCD_ENDPOINTS
value: "{{- $endpoints | join "," -}}"
-{{- end }}
\ No newline at end of file
+{{- end }}
+
+{{/*
+Generate data node names list based on passing roles
+*/}}
+{{- define "banyandb.dataNodeList" -}}
+{{- $dataNodes := list }}
+{{- $context := index . 0 }}
+{{- $configuredRoles := index . 1 }}
+{{- $fullname := include "banyandb.fullname" $context }}
+{{- range $roleName, $roleConfig := $context.Values.cluster.data.roles }}
+ {{- $shouldInclude := false }}
+ {{- if eq (len $configuredRoles) 0 }}
+ {{- $shouldInclude = true }}
+ {{- else }}
+ {{- if has $roleName $configuredRoles }}
+ {{- $shouldInclude = true }}
+ {{- end }}
+ {{- end }}
+ {{- if $shouldInclude }}
+ {{- $replicas := $roleConfig.replicas | default $context.Values.cluster.data.nodeTemplate.replicas }}
+ {{- range $i := until (int $replicas) }}
+ {{- $nodeName := printf "%s-data-%s-%d" $fullname $roleName $i }}
+ {{- $dataNodes = append $dataNodes $nodeName }}
+ {{- end }}
+ {{- end }}
+{{- end }}
+{{- $dataNodes | join "," -}}
+{{- end }}
Review Comment:
The suggestion looks tidier.
##########
chart/values.yaml:
##########
@@ -276,6 +276,12 @@ cluster:
## @param cluster.liaison.env Environment variables for liaison pods
##
env: []
+ ## Data node list environment configuration
+ ## @param cluster.liaison.dataNodeListEnv.enabled Enable BYDB_DATA_NODE_LIST environment variable
+ dataNodeListEnv:
+ enabled: false
+ ## @param cluster.liaison.dataNodeListEnv.roles List of data roles to include (empty list means all roles)
+ roles: []
Review Comment:
```suggestion
```
The env var is mandatory.
##########
chart/templates/_helpers.tpl:
##########
@@ -98,4 +98,32 @@ EtcdEndpoints
{{- end }}
- name: BYDB_ETCD_ENDPOINTS
value: "{{- $endpoints | join "," -}}"
-{{- end }}
\ No newline at end of file
+{{- end }}
+
+{{/*
+Generate data node names list based on passing roles
Review Comment:
This comment makes sense
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support data node list in the liaison [skywalking-banyandb-helm]
Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily merged PR #39:
URL: https://github.com/apache/skywalking-banyandb-helm/pull/39
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org