-
Notifications
You must be signed in to change notification settings - Fork 60
common: accept legacy namespace in changefeed ID JSON #4080
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,6 +16,7 @@ package common | |||||||||||||||||||
| import ( | ||||||||||||||||||||
| "bytes" | ||||||||||||||||||||
| "encoding/binary" | ||||||||||||||||||||
| "encoding/json" | ||||||||||||||||||||
| "regexp" | ||||||||||||||||||||
| "strconv" | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -160,6 +161,45 @@ type ChangeFeedDisplayName struct { | |||||||||||||||||||
| Keyspace string `json:"keyspace"` | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // changeFeedDisplayNameJSON is a compatibility shim for ChangeFeedDisplayName JSON encoding. | ||||||||||||||||||||
| // | ||||||||||||||||||||
| // Why: some historical TiCDC versions persisted the user-facing dimension as "namespace". | ||||||||||||||||||||
| // Newer TiCDC versions renamed it to "keyspace". Mixed-version upgrades/rollbacks must | ||||||||||||||||||||
| // not make changefeeds "disappear" (e.g., keyspace becomes empty and gets filtered out) | ||||||||||||||||||||
| // or become un-recreatable due to occupied meta keys. To keep metadata compatible across | ||||||||||||||||||||
| // versions, we accept both field names on unmarshal and emit both on marshal. | ||||||||||||||||||||
| type changeFeedDisplayNameJSON struct { | ||||||||||||||||||||
| Name string `json:"name"` | ||||||||||||||||||||
| Keyspace string `json:"keyspace,omitempty"` | ||||||||||||||||||||
| Namespace string `json:"namespace,omitempty"` | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // MarshalJSON emits both "keyspace" (newer) and "namespace" (legacy) so that either | ||||||||||||||||||||
| // field name can be used to read the metadata without an explicit migration step. | ||||||||||||||||||||
| func (r ChangeFeedDisplayName) MarshalJSON() ([]byte, error) { | ||||||||||||||||||||
| return json.Marshal(changeFeedDisplayNameJSON{ | ||||||||||||||||||||
| Name: r.Name, | ||||||||||||||||||||
| Keyspace: r.Keyspace, | ||||||||||||||||||||
| Namespace: r.Keyspace, | ||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we don't need to Marshal the Namespace field |
||||||||||||||||||||
| }) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // UnmarshalJSON accepts both "keyspace" (newer) and "namespace" (legacy). | ||||||||||||||||||||
| // If both are present, "keyspace" takes precedence. | ||||||||||||||||||||
| func (r *ChangeFeedDisplayName) UnmarshalJSON(data []byte) error { | ||||||||||||||||||||
| var decoded changeFeedDisplayNameJSON | ||||||||||||||||||||
| if err := json.Unmarshal(data, &decoded); err != nil { | ||||||||||||||||||||
| return err | ||||||||||||||||||||
| } | ||||||||||||||||||||
| r.Name = decoded.Name | ||||||||||||||||||||
| if decoded.Keyspace != "" { | ||||||||||||||||||||
| r.Keyspace = decoded.Keyspace | ||||||||||||||||||||
| return nil | ||||||||||||||||||||
| } | ||||||||||||||||||||
| r.Keyspace = decoded.Namespace | ||||||||||||||||||||
|
Comment on lines
+195
to
+199
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic for setting
Suggested change
|
||||||||||||||||||||
| return nil | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| func NewChangeFeedDisplayName(name string, keyspace string) ChangeFeedDisplayName { | ||||||||||||||||||||
| return ChangeFeedDisplayName{ | ||||||||||||||||||||
| Name: name, | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| // Copyright 2025 PingCAP, Inc. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package common | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| func TestChangeFeedDisplayNameJSONCompatibility(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // Scenario: TiCDC historically persisted changefeed IDs using "namespace". | ||
| // Newer versions renamed the dimension to "keyspace". We must be able to | ||
| // read both to avoid cross-version upgrades/rollbacks making changefeeds | ||
| // "disappear" (e.g., keyspace becomes empty and gets filtered out). | ||
|
|
||
| t.Run("unmarshal legacy namespace", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // Step: decode legacy JSON that only contains "namespace". | ||
| var got ChangeFeedDisplayName | ||
| require.NoError(t, json.Unmarshal([]byte(`{"name":"cf","namespace":"default"}`), &got)) | ||
|
|
||
| // Expect: Keyspace is filled from the legacy field. | ||
| require.Equal(t, ChangeFeedDisplayName{Name: "cf", Keyspace: "default"}, got) | ||
| }) | ||
|
|
||
| t.Run("unmarshal new keyspace", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // Step: decode newer JSON that only contains "keyspace". | ||
| var got ChangeFeedDisplayName | ||
| require.NoError(t, json.Unmarshal([]byte(`{"name":"cf","keyspace":"default"}`), &got)) | ||
|
|
||
| // Expect: Keyspace is filled from the new field. | ||
| require.Equal(t, ChangeFeedDisplayName{Name: "cf", Keyspace: "default"}, got) | ||
| }) | ||
|
|
||
| t.Run("keyspace wins when both exist", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // Step: decode mixed JSON produced/consumed by different versions. | ||
| var got ChangeFeedDisplayName | ||
| require.NoError(t, json.Unmarshal([]byte(`{"name":"cf","namespace":"ns","keyspace":"ks"}`), &got)) | ||
|
|
||
| // Expect: new field takes precedence to match current semantics. | ||
| require.Equal(t, ChangeFeedDisplayName{Name: "cf", Keyspace: "ks"}, got) | ||
| }) | ||
|
|
||
| t.Run("marshal emits both fields", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // Step: encode a display name. | ||
| data, err := json.Marshal(ChangeFeedDisplayName{Name: "cf", Keyspace: "default"}) | ||
| require.NoError(t, err) | ||
|
|
||
| // Expect: both fields exist so that either a legacy ("namespace") or a newer | ||
| // ("keyspace") TiCDC binary can read metadata without rewriting it first. | ||
| var got map[string]any | ||
| require.NoError(t, json.Unmarshal(data, &got)) | ||
| require.Equal(t, "cf", got["name"]) | ||
| require.Equal(t, "default", got["namespace"]) | ||
| require.Equal(t, "default", got["keyspace"]) | ||
| }) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rewriting the UnmarshalJSON method is significantly better than defining a new struct.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already rewrited UnmarshalJSON on ChangeFeedDisplayName. We keep the unexported changeFeedDisplayNameJSON shim only to share the JSON tags between MarshalJSON/UnmarshalJSON (to keep them symmetric and avoid duplication/drift) and to avoid unmarshalling twice.