invalidate stale regions for Mpp query. #1859
Conversation
|
Should we add this for batch coprocessor either? |
yeah, I plan to add it later |
| @@ -239,6 +239,9 @@ void DAGQueryBlockInterpreter::executeTS(const tipb::TableScan & ts, Pipeline & | |||
| // For those regions which are not presented in this tiflash node, we will try to fetch streams by key ranges from other tiflash nodes, only happens in batch cop mode. | |||
| if (!region_retry.empty()) | |||
| { | |||
| for (auto it: region_retry) { | |||
| retry_regions.push_back(it.second); | |||
There was a problem hiding this comment.
Why not just put the retry regions into DAGContext.retry_regions directly?
There was a problem hiding this comment.
The region_retry include two kinds of regions:
- region with epoch error
- region with lock error
do we need to return all of them or just return the ones with epoch error?
There was a problem hiding this comment.
we needn't. Is there an easy way to pick all the stale ones?
There was a problem hiding this comment.
I'm afraid there is no easy way since region_retry is only a set and does not keep the retry reason, you can refine it to record the retry reason alongside with the region id if the cost of invalidate a region everytime when it meet lock exception is unacceptable.
There was a problem hiding this comment.
I don't think the cost is too high. How about keeping it?
|
./build |
| @@ -239,6 +239,9 @@ void DAGQueryBlockInterpreter::executeTS(const tipb::TableScan & ts, Pipeline & | |||
| // For those regions which are not presented in this tiflash node, we will try to fetch streams by key ranges from other tiflash nodes, only happens in batch cop mode. | |||
| if (!region_retry.empty()) | |||
| { | |||
| for (auto it: region_retry) { | |||
| retry_regions.push_back(it.second); | |||
There was a problem hiding this comment.
The region_retry include two kinds of regions:
- region with epoch error
- region with lock error
do we need to return all of them or just return the ones with epoch error?
| if (dag_inter != nullptr) | ||
| { | ||
| context.getQueryContext().getDAGContext()->retry_regions = dag_inter->retry_regions; | ||
| } |
There was a problem hiding this comment.
since the retry regions are already put into DAGContext.retry_regions, why this piece of code is needed?
| @@ -248,6 +248,10 @@ void DAGQueryBlockInterpreter::executeTS(const tipb::TableScan & ts, Pipeline & | |||
| // For those regions which are not presented in this tiflash node, we will try to fetch streams by key ranges from other tiflash nodes, only happens in batch cop mode. | |||
| if (!region_retry.empty()) | |||
| { | |||
| for (auto it: region_retry) { | |||
|
/run-all-tests |
hanfei1991
added
the
needs-cherry-pick-release-5.0
|
/run-all-tests |
|
/merge |
ti-srebot
added
the
status/can-merge
|
/run-all-tests |
1 similar comment
|
/run-all-tests |
|
cherry pick to release-5.0 in PR #1877 |
What problem does this PR solve?
Problem Summary:
issue: #1873
Right now, we won't update the stale regions during executing Mpp queries. Then we added the try regions field in MppDispatchTask reponse and try to invalidate the stale regions on TiDB side.
What is changed and how it works?
Related changes
pingcap/kvproto#751
Check List
Tests
["DAGQueryBlockInterpreter: Start to retry 1 regions (10936,)"]["invalid region because tiflash detected stale region"] ["region id"="{ region id: 10936, ver: 116, confVer: 2 }"]proving this function works.Release note