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: port dag executor to endpoint #1975

Merged
merged 32 commits into from Jul 17, 2017

Conversation

Projects
None yet
6 participants
@lishihai9017
Contributor

lishihai9017 commented Jun 29, 2017

No description provided.

Show outdated Hide outdated src/server/coprocessor/dag.rs
return Err(box_err!("dag copr has not executor"));
}
// check whether first exec is *scan and get the column info
let first = &execs[0];

This comment has been minimized.

@andelf

andelf Jun 29, 2017

Contributor
let first = try!(self.req.get_executors().first()
    .ok_or_else(|| Err(box_err!("dag copr has not executor")))
);
@andelf

andelf Jun 29, 2017

Contributor
let first = try!(self.req.get_executors().first()
    .ok_or_else(|| Err(box_err!("dag copr has not executor")))
);
Show outdated Hide outdated src/server/coprocessor/dag.rs
first.get_tp()))
}
}
// check whether dag has a aggregation action and take a flag

This comment has been minimized.

@andelf

andelf Jun 29, 2017

Contributor

takes

@andelf

andelf Jun 29, 2017

Contributor

takes

This comment has been minimized.

@lishihai9017

lishihai9017 Jun 30, 2017

Contributor

no take method for tp

@lishihai9017

lishihai9017 Jun 30, 2017

Contributor

no take method for tp

This comment has been minimized.

@andelf

andelf Jun 30, 2017

Contributor

ok

@andelf

andelf Jun 30, 2017

Contributor

ok

Show outdated Hide outdated src/server/coprocessor/dag.rs
}
}
// check whether dag has a aggregation action and take a flag
if execs.iter().rfind(|&exec| exec.get_tp() == ExecType::TypeAggregation).is_some() {

This comment has been minimized.

@andelf

andelf Jun 29, 2017

Contributor

rev().find() to avoid feature gate?

@andelf

andelf Jun 29, 2017

Contributor

rev().find() to avoid feature gate?

Show outdated Hide outdated src/server/coprocessor/dag.rs
pub fn build_dag(&'s self, statistics: &'s mut Statistics) -> Result<Box<DAGExecutor + 's>> {
let mut execs = self.req.get_executors().to_vec().into_iter();
let mut src = self.build_first(execs.next().unwrap(), statistics);
for mut exec in execs {

This comment has been minimized.

@andelf

andelf Jun 29, 2017

Contributor

is Iterator::fold pattern solves?

@andelf

andelf Jun 29, 2017

Contributor

is Iterator::fold pattern solves?

This comment has been minimized.

@lishihai9017

lishihai9017 Jun 30, 2017

Contributor

yes, i had a try, but it makes code a little difficult to understand based on the origin logic which is already a little complicated.

@lishihai9017

lishihai9017 Jun 30, 2017

Contributor

yes, i had a try, but it makes code a little difficult to understand based on the origin logic which is already a little complicated.

Show outdated Hide outdated src/server/coprocessor/endpoint.rs
@@ -360,54 +379,98 @@ impl TiDbEndPoint {
on_error(e, t);
return;
}
match self.handle_select(&mut t) {
let res = match t.cop_req.take().unwrap() {

This comment has been minimized.

@andelf

andelf Jun 29, 2017

Contributor

res is a bad name for response. (result)
resp may be better.

@andelf

andelf Jun 29, 2017

Contributor

res is a bad name for response. (result)
resp may be better.

}
pub fn build_dag(&'s self, statistics: &'s mut Statistics) -> Result<Box<DAGExecutor + 's>> {
let mut execs = self.req.get_executors().to_vec().into_iter();

This comment has been minimized.

@hhkbp2

hhkbp2 Jul 6, 2017

Contributor

Is it possible to avoid copying here?

@hhkbp2

hhkbp2 Jul 6, 2017

Contributor

Is it possible to avoid copying here?

This comment has been minimized.

@lishihai9017

lishihai9017 Jul 7, 2017

Contributor

use reference here will generate a mut borrow conflict in the env which used this func.

@lishihai9017

lishihai9017 Jul 7, 2017

Contributor

use reference here will generate a mut borrow conflict in the env which used this func.

Show outdated Hide outdated src/coprocessor/endpoint.rs
sel_resp.set_error(to_pb_error(&e));
// TODO add detail error
resp.set_data(box_try!(sel_resp.write_to_bytes()));

This comment has been minimized.

@hhkbp2

hhkbp2 Jul 6, 2017

Contributor

Why set sel_resp on error?

@hhkbp2

hhkbp2 Jul 6, 2017

Contributor

Why set sel_resp on error?

This comment has been minimized.

@lishihai9017

lishihai9017 Jul 7, 2017

Contributor

feed back error happening in coprocessor executing to tidb

@lishihai9017

lishihai9017 Jul 7, 2017

Contributor

feed back error happening in coprocessor executing to tidb

Show outdated Hide outdated src/coprocessor/endpoint.rs
Error::Other(_) => {
resp.set_other_error(format!("{}", e));
COPR_REQ_ERROR.with_label_values(&["select", "other"]).inc();
}

This comment has been minimized.

@AndreMouche

AndreMouche Jul 7, 2017

Member

When will this error happen?

@AndreMouche

AndreMouche Jul 7, 2017

Member

When will this error happen?

This comment has been minimized.

@lishihai9017

lishihai9017 Jul 8, 2017

Contributor

box_try

@lishihai9017

lishihai9017 Jul 8, 2017

Contributor

box_try

let res = match t.req.get_tp() {
REQ_TYPE_SELECT => ctx.get_rows_from_sel(range),
REQ_TYPE_INDEX => ctx.get_rows_from_idx(range),
_ => unreachable!(),

This comment has been minimized.

@AndreMouche

AndreMouche Jul 7, 2017

Member

It seems the request type here should be REQ_TYPE_SELECT or REQ_TYPE_INDEX, so I prefer to use if .. else... And If we keep to use match, I think we should throw a panic instead of return unreachable.

@AndreMouche

AndreMouche Jul 7, 2017

Member

It seems the request type here should be REQ_TYPE_SELECT or REQ_TYPE_INDEX, so I prefer to use if .. else... And If we keep to use match, I think we should throw a panic instead of return unreachable.

This comment has been minimized.

@lishihai9017

lishihai9017 Jul 8, 2017

Contributor

using match in two branch instead of using if/else is more understandable, and use unreadchable is reasonable since the bug is explicit.

@lishihai9017

lishihai9017 Jul 8, 2017

Contributor

using match in two branch instead of using if/else is more understandable, and use unreadchable is reasonable since the bug is explicit.

Show outdated Hide outdated src/coprocessor/endpoint.rs
Err(e) => {
if let Error::Other(_) = e {
// should we handle locked here too?
let mut resp = Response::new();
let mut sel_resp = SelectResponse::new();

This comment has been minimized.

@AndreMouche

AndreMouche Jul 7, 2017

Member

Why repeat the same logic each branch? I prefer the previous implementation.

@AndreMouche

AndreMouche Jul 7, 2017

Member

Why repeat the same logic each branch? I prefer the previous implementation.

Show outdated Hide outdated src/coprocessor/dag.rs
pub fn validate_dag(&mut self) -> Result<()> {
let execs = self.req.get_executors();
let first = try!(execs.first()
.ok_or_else(|| Error::Other(box_err!("dag copr has not executor"))));

This comment has been minimized.

@AndreMouche

AndreMouche Jul 7, 2017

Member

has no executor

@AndreMouche

AndreMouche Jul 7, 2017

Member

has no executor

@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang

siddontang Jul 8, 2017

Contributor

any update?

Contributor

siddontang commented Jul 8, 2017

any update?

Show outdated Hide outdated src/coprocessor/dag.rs
snap: &'s Snapshot,
pub has_aggr: bool,
eval_ctx: Rc<EvalContext>,
pub chunks: Vec<Chunk>,

This comment has been minimized.

@AndreMouche

AndreMouche Jul 9, 2017

Member

Please reorder the members to make the public members together.

@AndreMouche

AndreMouche Jul 9, 2017

Member

Please reorder the members to make the public members together.

Show outdated Hide outdated src/coprocessor/dag.rs
pub fn validate_dag(&mut self) -> Result<()> {
let execs = self.req.get_executors();
let first = try!(execs.first()
.ok_or_else(|| Error::Other(box_err!("has not executor"))));

This comment has been minimized.

@AndreMouche

AndreMouche Jul 9, 2017

Member

has no executor

@AndreMouche

AndreMouche Jul 9, 2017

Member

has no executor

Show outdated Hide outdated src/coprocessor/endpoint.rs
sel.get_start_ts(),
t.req.get_context().get_isolation_level());
pub fn handle_select(&self, sel: SelectRequest, t: &mut RequestTask) -> Result<Response> {
let snap = SnapshotStore::new(self.snap.as_ref(), sel.get_start_ts(), IsolationLevel::SI);

This comment has been minimized.

@AndreMouche

AndreMouche Jul 9, 2017

Member

I think the isolation level should be get from request context.

@AndreMouche

AndreMouche Jul 9, 2017

Member

I think the isolation level should be get from request context.

for mut exec in execs {
let curr: Box<DAGExecutor> = match exec.get_tp() {
ExecType::TypeTableScan | ExecType::TypeIndexScan => {
return Err(box_err!("got too much *scan exec, should be only one"))

This comment has been minimized.

@AndreMouche

AndreMouche Jul 9, 2017

Member

tablescan and indexscan should only be in the head of the queue?

@AndreMouche

AndreMouche Jul 9, 2017

Member

tablescan and indexscan should only be in the head of the queue?

This comment has been minimized.

@lishihai9017

lishihai9017 Jul 10, 2017

Contributor

yes, it's guaranteed by upper layer

@lishihai9017

lishihai9017 Jul 10, 2017

Contributor

yes, it's guaranteed by upper layer

let mut execs = self.req.get_executors().to_vec().into_iter();
let mut src = self.build_first(execs.next().unwrap(), statistics);
for mut exec in execs {
let curr: Box<DAGExecutor> = match exec.get_tp() {

This comment has been minimized.

@AndreMouche

AndreMouche Jul 9, 2017

Member

What if set src directly here?

@AndreMouche

AndreMouche Jul 9, 2017

Member

What if set src directly here?

resp.set_data(box_try!(sel_resp.write_to_bytes()));
resp.set_other_error(format!("{}", e));
return Ok(resp);
} else {

This comment has been minimized.

@AndreMouche

AndreMouche Jul 9, 2017

Member

What if meet a lock?

@AndreMouche

AndreMouche Jul 9, 2017

Member

What if meet a lock?

This comment has been minimized.

@lishihai9017

lishihai9017 Jul 10, 2017

Contributor

we only feed back the other error which means undefined from the original.

@lishihai9017

lishihai9017 Jul 10, 2017

Contributor

we only feed back the other error which means undefined from the original.

Show outdated Hide outdated src/coprocessor/endpoint.rs
return Err(e);
}
}
}
let data = box_try!(sel_resp.write_to_bytes());
resp.set_data(data);
return Ok(resp);

This comment has been minimized.

@AndreMouche

AndreMouche Jul 9, 2017

Member

remove return as shown: Ok(resp). Please run make dev before git commit

@AndreMouche

AndreMouche Jul 9, 2017

Member

remove return as shown: Ok(resp). Please run make dev before git commit

Show outdated Hide outdated src/coprocessor/dag.rs
}
}
// check whether dag has a aggregation action and take a flag
if execs.iter().rev().any(|ref exec| exec.get_tp() == ExecType::TypeAggregation) {

This comment has been minimized.

@AndreMouche

AndreMouche Jul 9, 2017

Member

needless borrow for exec

@AndreMouche

AndreMouche Jul 9, 2017

Member

needless borrow for exec

@AndreMouche

This comment has been minimized.

Show comment
Hide comment
@AndreMouche

AndreMouche Jul 12, 2017

Member

Please resolve conflicts.

Member

AndreMouche commented Jul 12, 2017

Please resolve conflicts.

@AndreMouche

This comment has been minimized.

Show comment
Hide comment
Member

AndreMouche commented Jul 12, 2017

@AndreMouche

This comment has been minimized.

Show comment
Hide comment
@AndreMouche

AndreMouche Jul 14, 2017

Member

Please resolve conflicts to offer the reviewers convenience

Member

AndreMouche commented Jul 14, 2017

Please resolve conflicts to offer the reviewers convenience

@@ -1 +1,14 @@
// Copyright 2016 PingCAP, Inc.

This comment has been minimized.

@hhkbp2

hhkbp2 Jul 17, 2017

Contributor

s/2016/2017/?

@hhkbp2

hhkbp2 Jul 17, 2017

Contributor

s/2016/2017/?

This comment has been minimized.

@lishihai9017

lishihai9017 Jul 17, 2017

Contributor

the file created at 2016.

@lishihai9017

lishihai9017 Jul 17, 2017

Contributor

the file created at 2016.

@@ -1,23 +1,39 @@
// Copyright 2016 PingCAP, Inc.

This comment has been minimized.

@hhkbp2

hhkbp2 Jul 17, 2017

Contributor

Dito.

@hhkbp2

hhkbp2 Jul 17, 2017

Contributor

Dito.

Show outdated Hide outdated tests/coprocessor/test_select.rs
@@ -83,6 +99,26 @@ impl Iterator for ChunkSpliter {
}
}
#[allow(dead_code)]

This comment has been minimized.

@hhkbp2

hhkbp2 Jul 17, 2017

Contributor

Why keep this function?

@hhkbp2

hhkbp2 Jul 17, 2017

Contributor

Why keep this function?

@shenli

This comment has been minimized.

Show comment
Hide comment
@shenli

shenli Jul 17, 2017

Member

@lishihai9017 Please resolve the conflicts.

Member

shenli commented Jul 17, 2017

@lishihai9017 Please resolve the conflicts.

Show outdated Hide outdated tests/coprocessor/test_select.rs
self.build_with(&[0])
}
fn build_with(mut self, flags: &[u64]) -> Request {
fn build_with(mut self, flags: &[u64]) -> (Request, Request) {

This comment has been minimized.

@hhkbp2

hhkbp2 Jul 17, 2017

Contributor

Add a comment for this function. (Request, Request) is not a returned value which is easy to understand.

@hhkbp2

hhkbp2 Jul 17, 2017

Contributor

Add a comment for this function. (Request, Request) is not a returned value which is easy to understand.

@hhkbp2

This comment has been minimized.

Show comment
Hide comment
@hhkbp2

hhkbp2 Jul 17, 2017

Contributor

Rest LGTM

Contributor

hhkbp2 commented Jul 17, 2017

Rest LGTM

@hhkbp2

This comment has been minimized.

Show comment
Hide comment
@hhkbp2

hhkbp2 Jul 17, 2017

Contributor

LGTM

Contributor

hhkbp2 commented Jul 17, 2017

LGTM

@hhkbp2

hhkbp2 approved these changes Jul 17, 2017

@lishihai9017 lishihai9017 merged commit 8574c82 into master Jul 17, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@tikv tikv deleted a comment from AndreMouche Jul 17, 2017

@lishihai9017 lishihai9017 deleted the lishihai/dag-exec-porting branch Jul 17, 2017

@tikv tikv deleted a comment from AndreMouche Jul 17, 2017

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