Skip to content
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

model: implement UnmarshalJSON for CIStr #7252

Merged
merged 3 commits into from
Aug 2, 2018

Conversation

tiancaiamao
Copy link
Contributor

What have you changed? (mandatory)

So it can be unmarshaled from a single string

What is the type of the changes? (mandatory)

  • Improvement (non-breaking change which is an improvement to an existing feature)

How has this PR been tested? (mandatory)

unit test

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

no

Does this PR affect tidb-ansible update? (mandatory)

no

Does this PR need to be added to the release notes? (mandatory)

no

Refer to a related PR or issue link (optional)

no

Benchmark result if necessary (optional)

I'll add one

Add a few positive/negative examples (optional)

Upgrade from 2.0 to master may meets some problems if partition tables are used.
Because master use

// PartitionDefinition defines a single partition.
type PartitionDefinition struct {
	ID       int64    `json:"id"`
	Name     CIStr    `json:"name"`
	LessThan []string `json:"less_than"`
	Comment  string   `json:"comment,omitempty"`
}

while 2.0 use

// PartitionDefinition defines a single partition.
type PartitionDefinition struct {
	ID       int64    `json:"id"`
	Name     string    `json:"name"`
	LessThan []string `json:"less_than"`
	Comment  string   `json:"comment,omitempty"`
}

After this PR, the upgrade would be smooth.

@winkyao @ciscoxll @shenli

So it can be unmarshaled from a single string
model/model.go Outdated
return nil
}

// Unmarshal CIStr from a single string.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add some comment, explain why we need to unmarshal from a single string.

@ciscoxll
Copy link
Contributor

ciscoxll commented Aug 2, 2018

/run-all-tests

@ciscoxll
Copy link
Contributor

ciscoxll commented Aug 2, 2018

LGTM

@ciscoxll ciscoxll added status/all tests passed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 2, 2018
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@coocood coocood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@coocood
Copy link
Member

coocood commented Aug 2, 2018

/run-all-tests

@ciscoxll ciscoxll added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 2, 2018
@ciscoxll ciscoxll merged commit 270ed29 into pingcap:master Aug 2, 2018
@tiancaiamao tiancaiamao deleted the model-cistr branch August 2, 2018 08:59
jackysp pushed a commit that referenced this pull request Aug 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants