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

Adds functions to build batch executors #4243

Merged
merged 85 commits into from Mar 6, 2019

Conversation

Projects
None yet
4 participants
@breeswish
Copy link
Member

breeswish commented Feb 20, 2019

What have you changed? (mandatory)

This PR adds functions to build batch executors, which will build executors to run on batch executor handlers.

Extracted from #3898

What are the type of the changes? (mandatory)

  • New Feature

breeswish added some commits Feb 14, 2019

Extract batch column changes
Signed-off-by: Breezewish <breezewish@pingcap.com>
Rename BatchColumn to VectorValue
Signed-off-by: Breezewish <breezewish@pingcap.com>
Extract clone_vec_with_capacity into util
Signed-off-by: Breezewish <breezewish@pingcap.com>
Extract LazyBatchColumn
Signed-off-by: Breezewish <breezewish@pingcap.com>
Rename BatchRows to LazyBatchColumnVec
Signed-off-by: Breezewish <breezewish@pingcap.com>
- Remove error from LazyBatchColumnVec
- Implement Deref for LazyBatchColumnVec
- Remove `get_` from `get_xxx` for LazyBatchColumn

Signed-off-by: Breezewish <breezewish@pingcap.com>
Merge branch '___batch_extract/batch_column_4' into ___batch_extract/…
…batch_column

Signed-off-by: Breezewish <breezewish@pingcap.com>
2018 -> 2019
Signed-off-by: Breezewish <breezewish@pingcap.com>
Implement encode functions for VectorValue and LazyBatchColumn(Vec)
Signed-off-by: Breezewish <breezewish@pingcap.com>
Merge remote-tracking branch 'origin/master' into ___batch_extract/ba…
…tch_column_1

Signed-off-by: Breezewish <breezewish@pingcap.com>
Merge branch '___batch_extract/batch_column_1' into ___batch_extract/…
…batch_column_2

Signed-off-by: Breezewish <breezewish@pingcap.com>
Address comments in former PRs
Signed-off-by: Breezewish <breezewish@pingcap.com>
Fix typo
Signed-off-by: Breezewish <breezewish@pingcap.com>
Merge remote-tracking branch 'origin/master' into ___batch_extract/ba…
…tch_column_4

Signed-off-by: Breezewish <breezewish@pingcap.com>
Merge commit '55eff8a5fb3a7b447cf987120edf2b80a9ad8e7f' into ___batch…
…_extract/batch_column_5

Signed-off-by: Breezewish <breezewish@pingcap.com>
Address comments about some typos and grammar
Signed-off-by: Breezewish <breezewish@pingcap.com>
Provide ScalarValue
Signed-off-by: Breezewish <breezewish@pingcap.com>

AndreMouche and others added some commits Feb 26, 2019

Add comments for `pub` fields and structures
Signed-off-by: Breezewish <breezewish@pingcap.com>
Fix typo
Signed-off-by: Breezewish <breezewish@pingcap.com>
Add TODOs for two major concerns during review, so that we can continue.
Signed-off-by: Breezewish <breezewish@pingcap.com>
Merge branch '___batch_extract/batch_column_t2/3' into ___batch_extra…
…ct/batch_column_t2/4

Signed-off-by: Breezewish <breezewish@pingcap.com>
Fix dyn
Signed-off-by: Breezewish <breezewish@pingcap.com>
fix dyn
Signed-off-by: Breezewish <breezewish@pingcap.com>
Merge commit 'e2e9625a396cee537f7f9e83631452361e6f3caa' into ___batch…
…_extract/batch_column_t2/3

Signed-off-by: Breezewish <breezewish@pingcap.com>
Add deadline and more comments
Signed-off-by: Breezewish <breezewish@pingcap.com>
Adds several TODOs for these possible tuning works
Signed-off-by: Breezewish <breezewish@pingcap.com>
Add deadline support
Signed-off-by: Breezewish <breezewish@pingcap.com>
@breeswish

This comment has been minimized.

Copy link
Member Author

breeswish commented Mar 1, 2019

/run-integration-tests

@AndreMouche
Copy link
Member

AndreMouche left a comment

Rest LGTM

@@ -139,6 +139,8 @@ impl<E: Engine> Endpoint<E> {
store,
req_ctx.deadline,
batch_row_limit,
is_streaming,
true,

This comment has been minimized.

@AndreMouche

AndreMouche Mar 1, 2019

Member

I think we should make it configurable

This comment has been minimized.

@breeswish

breeswish Mar 1, 2019

Author Member

Yes, that's exactly the purpose of this flag. Before 3.0 release we should make it configurable and enables it by default (but disables for some critical users). It is easy to change it in the future. For now, let's just use this value because we want integration tests to test it.

})
}

fn build_batch_dag<S: Store + 'static>(

This comment has been minimized.

@rleungx

rleungx Mar 5, 2019

Member

Is there any way to still use build function for both DAGRequestHandler and BatchDAGHandler, and let the caller decide which handler should be used?

This comment has been minimized.

@breeswish

breeswish Mar 5, 2019

Author Member

I don't think it's possible, because batch executor and non-batch executors are different executors, and they use different DAG handlers. Once executors are built, there is no way to switch to another (unless you rebuild it). I suggest to decide this before building. There is a flag called enable_batch_if_possible that can forcely disables batch execution dynamically in future. See @AndreMouche 's comment.

@rleungx

rleungx approved these changes Mar 5, 2019

Copy link
Member

rleungx left a comment

LGTM.

@AndreMouche
Copy link
Member

AndreMouche left a comment

LGTM

@AndreMouche AndreMouche merged commit 7ae2fe0 into tikv:master Mar 6, 2019

2 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.