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,store/tikv: SelectDAG function parameter refactor #4645

Merged
merged 8 commits into from Sep 27, 2017

Conversation

Projects
None yet
4 participants
@tiancaiamao
Contributor

tiancaiamao commented Sep 26, 2017

  1. move some distsql.SelectDAG parameter to kv.Request struct
  2. modify tikv.RPCContext struct, remove kvrpcpb.Context in it
  3. let tikvrpc.Request struct share Context with its subcommand request

@coocood @disksing @hanfei1991

distsql,store/tikv: SelectDAG function parameter refactor
1. move some distsql.SelectDAG parameter to kv.Request struct
2. modify tikv.RPCContext struct, remove kvrpcpb.Context in it
3. let tikvrpc.Request struct share Context with its subcommand request
@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Sep 26, 2017

Member

What's the benefit to share Context with its subcommand request?

Member

coocood commented Sep 26, 2017

What's the benefit to share Context with its subcommand request?

tiancaiamao added some commits Sep 26, 2017

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Sep 26, 2017

Member

LGTM

Member

coocood commented Sep 26, 2017

LGTM

Show outdated Hide outdated distsql/distsql.go Outdated
@tiancaiamao

This comment has been minimized.

Show comment
Hide comment
@tiancaiamao

tiancaiamao Sep 27, 2017

Contributor

/run-all-tests

Contributor

tiancaiamao commented Sep 27, 2017

/run-all-tests

@zimulala

This comment has been minimized.

Show comment
Hide comment
@zimulala

zimulala Sep 27, 2017

Member

LGTM

Member

zimulala commented Sep 27, 2017

LGTM

e.result, err = distsql.SelectDAG(e.ctx.GetClient(), goctx.Background(), e.dagPB, kvRanges, e.ctx.GetSessionVars().DistSQLScanConcurrency, e.keepOrder, e.desc, getIsolationLevel(e.ctx.GetSessionVars()), e.priority)
var builder requestBuilder
kvReq, err := builder.SetTableRanges(e.tableID, e.ranges).
SetDAGRequest(e.dagPB).

This comment has been minimized.

@AndreMouche

AndreMouche Sep 27, 2017

Member

Could we init kvReq with e directly in one single function?

@AndreMouche

AndreMouche Sep 27, 2017

Member

Could we init kvReq with e directly in one single function?

This comment has been minimized.

@tiancaiamao

tiancaiamao Sep 27, 2017

Contributor

They are nearly the same but not identical...such as builder.SetTableRanges() and builder.SetTableHandles(), so they can't be written into one single function.
Through it's not one single function, you can view it as a single line:

builder.SetXXX().SetYYY().SetZZZ().Build()
@tiancaiamao

tiancaiamao Sep 27, 2017

Contributor

They are nearly the same but not identical...such as builder.SetTableRanges() and builder.SetTableHandles(), so they can't be written into one single function.
Through it's not one single function, you can view it as a single line:

builder.SetXXX().SetYYY().SetZZZ().Build()
Show outdated Hide outdated server/region_handler.go Outdated

@coocood coocood merged commit 5200745 into master Sep 27, 2017

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci Your tests passed on CircleCI!
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@coocood coocood deleted the tiancaiamao/refactor-select-dag branch Sep 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment