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

filter: update metric schema name #324

Merged
merged 2 commits into from
Mar 15, 2020
Merged

filter: update metric schema name #324

merged 2 commits into from
Mar 15, 2020

Conversation

lichunzhu
Copy link
Contributor

@lichunzhu lichunzhu commented Mar 14, 2020

What problem does this PR solve?

https://github.com/pingcap/tidb/blob/6c67561ee0df8c8aed205d0c26eff6ed3bedd3a4/util/misc.go#L150
Current tidb uses METRICS_SCHEMA instead of METRIC_SCHEMA.

What is changed and how it works?

Update metrics schema name.

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Breaking backward compatibility

@lichunzhu lichunzhu changed the title update metric schema name filter: update metric schema name Mar 14, 2020
Copy link
Contributor

@july2993 july2993 left a comment

Choose a reason for hiding this comment

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

lgtm

need update test too

Copy link
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

LGTM after fixing the test.

[2020-03-14T02:16:46.952Z] schema_test.go:49:

[2020-03-14T02:16:46.952Z]     c.Assert(IsSystemSchema(t.name), Equals, t.expected, Commentf("schema name = %s", t.name))

[2020-03-14T02:16:46.952Z] ... obtained bool = false

[2020-03-14T02:16:46.952Z] ... expected bool = true

[2020-03-14T02:16:46.952Z] ... schema name = METRIC_SCHEMA

@kennytm
Copy link
Contributor

kennytm commented Mar 14, 2020

The rename was introduced by pingcap/tidb#14874. Temporarily marking it DNM until we confirm if TiDB really wants to break compatibility between 3.x and 4.x.

@wjhuang2016
Copy link
Member

TiDB 3.0 and 3.1 don't have this schema, so it's safe to rename it.

@kennytm
Copy link
Contributor

kennytm commented Mar 14, 2020

Seems to be accidentally cherry-picked by pingcap/tidb#14202? @wjhuang2016 Perhaps remove their definitions from 3.0 and 3.1 to avoid accidental usage?

@wjhuang2016
Copy link
Member

@lonng PTAL

Copy link

@lonng lonng left a comment

Choose a reason for hiding this comment

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

LGTM

@kennytm kennytm merged commit 6bea09b into pingcap:master Mar 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants