Support initialization store labels #2945
Conversation
| } else { | ||
| storeLabels = map[string]string{} | ||
| } | ||
| storeLabels["groupName"] = tg.Name |
There was a problem hiding this comment.
For each TiKVGroup instance, we will set groupName: tikvgroup-name as its store label. @rleungx
|
|
||
| // InitializeStoreLabels indicates the store label set during initialization | ||
| // +optional | ||
| InitializeStoreLabels map[string]string `json:"initializeStoreLabels,omitempty"` |
There was a problem hiding this comment.
I would prefer a shorter name, storeLabels or initStorelabels
There was a problem hiding this comment.
maybe we should this generic way to configure environments for containers
https://github.com/pingcap/tidb-operator/blob/master/pkg/apis/pingcap/v1alpha1/types.go#L711
There was a problem hiding this comment.
Setting store label by ENV is the way operator pass the store labels for tikv but not obvious and convenient for users, I still suggest exposing store labels by a certain atrribute.
There was a problem hiding this comment.
I don't think it's necessary to provide a special way for this. It's simple as configuring environments for a normal pod in Kubernetes.
There was a problem hiding this comment.
We already have too many special configurations. Anyone knowing the standard way to configure environments for containers can configure this env and any other environments for TiKV and other components.
If we use this special field for STORE_LABELS, we have more things to consider, e.g. what if users set both STORE_LABELS env and storeLabels field. Should we merge them? If not, which one has a higher priority, etc. If we have a special field for this env, should we add fields for other envs if users ask?
There was a problem hiding this comment.
ok, I agree with setting store labels by env.
| if len(labels) < 1 { | ||
| return "" | ||
| } | ||
| s := "" | ||
| var keys []string | ||
| for k := range labels { | ||
| keys = append(keys, k) | ||
| } | ||
| sort.Strings(keys) | ||
| for _, key := range keys { | ||
| v, ok := labels[key] | ||
| if ok { | ||
| s = fmt.Sprintf("%s,%s", s, fmt.Sprintf("%s=%s", key, v)) | ||
| } | ||
| } | ||
| return s[1:] |
There was a problem hiding this comment.
| if len(labels) < 1 { | |
| return "" | |
| } | |
| s := "" | |
| var keys []string | |
| for k := range labels { | |
| keys = append(keys, k) | |
| } | |
| sort.Strings(keys) | |
| for _, key := range keys { | |
| v, ok := labels[key] | |
| if ok { | |
| s = fmt.Sprintf("%s,%s", s, fmt.Sprintf("%s=%s", key, v)) | |
| } | |
| } | |
| return s[1:] | |
| var envs []string | |
| for k, v := range labels { | |
| envs = append(envs, fmt.Sprintf("%s=%s", k, v)) | |
| } | |
| sort.Strings(envs) | |
| return strings.Join(envs, ",") |
we should always try to use built-in functions to simplify code
btw, please add some basic units 1) nil map 2) 1 element map 3) more than 1 elements map
What problem does this PR solve?
Close #2692
What is changed and how does it work?
Support initialization store labels for TiKV
Related changes
Does this PR introduce a user-facing change?: