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

distsql: Unify endian for Chunk encode format #13349

Merged
merged 18 commits into from
Nov 27, 2019
Merged

Conversation

wshwsh12
Copy link
Contributor

What problem does this PR solve?

When using chunk encode format for response , maybe the endian is different in tidb and tikv, and tidb can't decode the data.

What is changed and how it works?

Unify endian for Chunk encode format.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)

Code changes

Side effects

  • Increased code complexity

Related changes

Release note

Signed-off-by: wshwsh12 <793703860@qq.com>
Signed-off-by: wshwsh12 <793703860@qq.com>
Signed-off-by: wshwsh12 <793703860@qq.com>
Signed-off-by: wshwsh12 <793703860@qq.com>
@zyxbest
Copy link
Contributor

zyxbest commented Nov 11, 2019

/build

@codecov
Copy link

codecov bot commented Nov 12, 2019

Codecov Report

Merging #13349 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #13349   +/-   ##
===========================================
  Coverage   80.1863%   80.1863%           
===========================================
  Files           474        474           
  Lines        116753     116753           
===========================================
  Hits          93620      93620           
  Misses        15761      15761           
  Partials       7372       7372

@wshwsh12
Copy link
Contributor Author

/run-all-tests

@@ -169,5 +170,35 @@ func canUseChunkRPC(ctx sessionctx.Context) bool {
if ctx.GetSessionVars().EnableStreaming {
return false
}
if !supportedAlignment() {
Copy link
Contributor

Choose a reason for hiding this comment

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

We may check this when process starting and cache the result, instead of checking in each session.


// SetSystemEndian sets the system endian for the DAGRequest.
func SetSystemEndian(dagReq *tipb.DAGRequest) {
dagReq.TidbSystemEndian = GetSystemEndian()
Copy link
Contributor

Choose a reason for hiding this comment

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

We may check this when process starting and cache the result, instead of checking in each session.

distsql/request_builder_test.go Outdated Show resolved Hide resolved
distsql/distsql.go Outdated Show resolved Hide resolved
distsql/distsql.go Outdated Show resolved Hide resolved
distsql/distsql.go Outdated Show resolved Hide resolved
distsql/distsql.go Outdated Show resolved Hide resolved
@SunRunAway
Copy link
Contributor

And PTAL at the CI failure.

@wshwsh12
Copy link
Contributor Author

And PTAL at the CI failure.

The CI fail is because tipb hasn't been merged to master and update.

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

@SunRunAway SunRunAway changed the title *: Unify endian for Chunk encode format distsql: Unify endian for Chunk encode format Nov 26, 2019
@SunRunAway SunRunAway added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 26, 2019
XuHuaiyu
XuHuaiyu previously approved these changes Nov 26, 2019
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

LGTM

@wshwsh12 wshwsh12 added status/LGT2 Indicates that a PR has LGTM 2. and removed status/DNM status/LGT1 Indicates that a PR has LGTM 1. labels Nov 27, 2019
@XuHuaiyu XuHuaiyu added the status/can-merge Indicates a PR has been approved by a committer. label Nov 27, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 27, 2019

/run-all-tests

@sre-bot sre-bot merged commit 134b80c into pingcap:master Nov 27, 2019
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
@wshwsh12 wshwsh12 deleted the endian branch April 21, 2020 05:07
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants