-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
ddl_notifier: implement serialization and deserialization for SchemaChangeEvent
#56089
ddl_notifier: implement serialization and deserialization for SchemaChangeEvent
#56089
Conversation
Hi @fzzf678. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #56089 +/- ##
=================================================
- Coverage 72.8966% 56.7927% -16.1040%
=================================================
Files 1609 1762 +153
Lines 447141 635609 +188468
=================================================
+ Hits 325951 360980 +35029
- Misses 101132 249795 +148663
- Partials 20058 24834 +4776
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@@ -33,7 +35,7 @@ func TestPublishToTableStore(t *testing.T) { | |||
CREATE TABLE ddl_notifier ( | |||
ddl_job_id BIGINT, | |||
multi_schema_change_seq BIGINT COMMENT '-1 if the schema change does not belong to a multi-schema change DDL. 0 or positive numbers representing the sub-job index of a multi-schema change DDL', | |||
schema_change JSON COMMENT 'SchemaChange at rest', | |||
schema_change LONGBLOB COMMENT 'SchemaChange at rest', |
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.
What's the reason to change the column type?
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 tried the JSON
type and use row.GetJSON()
and row.GetBytes()
get the serialized data, but it can't be deserialized. I took a look at other system tables like tidb_global_task
and tidb_background_subtask
, they use LONGBLOB
type save the meta and row.GetBytes()
get the serialized data and I did so. Is it my way of operating the JSON
type is wrong?
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.
diff --git a/pkg/ddl/notifier/publish_testkit_test.go b/pkg/ddl/notifier/publish_testkit_test.go
index 30f0c1170f..d5e01cbabf 100644
--- a/pkg/ddl/notifier/publish_testkit_test.go
+++ b/pkg/ddl/notifier/publish_testkit_test.go
@@ -35,7 +35,7 @@ func TestPublishToTableStore(t *testing.T) {
CREATE TABLE ddl_notifier (
ddl_job_id BIGINT,
multi_schema_change_seq BIGINT COMMENT '-1 if the schema change does not belong to a multi-schema change DDL. 0 or positive numbers representing the sub-job index of a multi-schema change DDL',
- schema_change LONGBLOB COMMENT 'SchemaChange at rest',
+ schema_change JSON COMMENT 'SchemaChange at rest',
processed_by_flag BIGINT UNSIGNED DEFAULT 0 COMMENT 'flag to mark which subscriber has processed the event',
PRIMARY KEY(ddl_job_id, multi_schema_change_seq)
)
diff --git a/pkg/ddl/notifier/store.go b/pkg/ddl/notifier/store.go
index c652fd0052..f259277e0b 100644
--- a/pkg/ddl/notifier/store.go
+++ b/pkg/ddl/notifier/store.go
@@ -94,7 +94,12 @@ func (t *tableStore) List(ctx context.Context, se *sess.Session, limit int) ([]*
ret := make([]*schemaChange, 0, len(rows))
for _, row := range rows {
event := SchemaChangeEvent{}
- err = json.Unmarshal(row.GetBytes(2), &event)
+ binaryJSON := row.GetJSON(2)
+ jsonStr, err := binaryJSON.MarshalJSON()
+ if err != nil {
+ return nil, errors.Trace(err)
+ }
+ err = json.Unmarshal(jsonStr, &event)
if err != nil {
return nil, errors.Trace(err)
}
However I don't have strong opinion about the column type. JSON type can add JSON syntax check in the SQL layer and readable SELECT output, LONGTEXT or LONGBLOB has readable SELECT output. LONGBLOB has no advantage. Maybe wait other reviewers' opinions.
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 tried again and found that the JSON
type in table is not equal to our internal json
type in code, we need a conversion to use the JSON
type in table like this:
j, err := row.GetJSON(2).MarshalJSON()
err = json.Unmarshal(j, &event)
https://github.com/pingcap/tidb/blob/master/pkg/types/json_binary.go#L151-L180
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.
rest lgtm
@@ -33,7 +35,7 @@ func TestPublishToTableStore(t *testing.T) { | |||
CREATE TABLE ddl_notifier ( | |||
ddl_job_id BIGINT, | |||
multi_schema_change_seq BIGINT COMMENT '-1 if the schema change does not belong to a multi-schema change DDL. 0 or positive numbers representing the sub-job index of a multi-schema change DDL', | |||
schema_change JSON COMMENT 'SchemaChange at rest', | |||
schema_change LONGBLOB COMMENT 'SchemaChangeEvent at rest', |
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.
Because other reviewers and me has no comment, I think you can choose the type as you like. Please /unhold
after you choose it.
/hold |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CbcWestwolf, lance6716 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
/retest |
/unhold |
@fzzf678: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What problem does this PR solve?
Issue Number: ref #55722
Problem Summary:
What changed and how does it work?
Add
Marshal
andUnMarshal
method forSchemaChangeEvent
.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.