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

coprocessor: add ddl schema version in request #14317

Merged
merged 9 commits into from
Jan 8, 2020

Conversation

lzmhhh123
Copy link
Contributor

What problem does this PR solve?

When DDL has changed. TiFlash needs to know the DDL change to decode the correct data. So for TiFlash, it hopes TiDB lets it know the newest schema version in coprocessor request.

What is changed and how it works?

Set the SchemaVer for coprocessor.Request.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has persistent data change

Side effects

  • Possible performance regression
  • Increased code complexity

@lzmhhh123 lzmhhh123 added type/enhancement The issue or PR belongs to an enhancement. component/coprocessor labels Jan 2, 2020
@lzmhhh123
Copy link
Contributor Author

/run-all-tests

@lzmhhh123 lzmhhh123 force-pushed the dev/add_schema_ver_in_cop branch from 82811ad to 1f41051 Compare January 8, 2020 03:08
@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 8, 2020
Copy link
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -165,6 +165,7 @@ func (builder *RequestBuilder) SetFromSessionVars(sv *variable.SessionVars) *Req
builder.Request.NotFillCache = sv.StmtCtx.NotFillCache
builder.Request.Priority = builder.getKVPriority(sv)
builder.Request.ReplicaRead = sv.GetReplicaRead()
builder.Request.SchemaVar = sv.TxnCtx.SchemaVersion
Copy link
Member

Choose a reason for hiding this comment

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

Please update the comment of this function.

Copy link
Member

Choose a reason for hiding this comment

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

Emmm, please update Priority in comment too.

@SunRunAway SunRunAway added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 8, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jan 8, 2020

/run-all-tests

@sre-bot sre-bot merged commit 07e642c into pingcap:master Jan 8, 2020
@lzmhhh123 lzmhhh123 deleted the dev/add_schema_ver_in_cop branch January 8, 2020 08:31
lzmhhh123 added a commit to lzmhhh123/tidb that referenced this pull request Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/coprocessor status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants