Support blocklisting TiFlash WN node#198
Conversation
include/pingcap/coprocessor/Client.h
Outdated
| bool is_partition_table_scan, | ||
| const std::vector<int64_t> & physical_table_ids, | ||
| const std::vector<KeyRanges> & ranges_for_each_physical_table, | ||
| const std::unordered_set<uint64_t> * store_id_blacklist, |
There was a problem hiding this comment.
IMO store_id_blocklist is better.
There was a problem hiding this comment.
OK, I got your point here...
There was a problem hiding this comment.
Add some test cases about the new filter logic
src/kv/RegionCache.cc
Outdated
| StoreNotReady)); | ||
| continue; | ||
| } | ||
| if (store_id_blacklist && store_id_blacklist->count(store.id) > 0) { |
src/kv/RegionCache.cc
Outdated
| if (cached_region == nullptr) | ||
| { | ||
| all_stores.emplace_back(current_store.id); | ||
| remove_blacklist(store_id_blacklist, all_stores, region_id, log); |
There was a problem hiding this comment.
After you added this logic, the function RegionCache::getAllValidTiFlashStores now may return all_stores with an empty set? Can the caller handle this situation?
There was a problem hiding this comment.
Yes, it is tested in https://github.com/tidbcloud/tiflash-cse/pull/308 where none of the two wn nodes can be selected. Basicly, getAllValidTiFlashStores is to get all valid stores to do some balance work, whereas the "effective" store is returned by getRPCContext
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
|
@guo-shaoge: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| if (cached_region == nullptr) | ||
| { | ||
| all_stores.emplace_back(current_store.id); | ||
| remove_blocklist(store_id_blocklist, all_stores, region_id, log); |
There was a problem hiding this comment.
balanceBatchCopTasks may meet out-of-index in this line.
client-c/src/coprocessor/Client.cc
Line 158 in 617d233
There was a problem hiding this comment.
In buildBatchCopTasks, if no store can be selected, it won't go that far. It will throw an exception in backoffer because getRPCContext returns none.
There was a problem hiding this comment.
I have add an Exception here. Moreover, the most common path is is_mpp=true
|
@JaySon-Huang: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gengliqi, guo-shaoge, JaySon-Huang, LykxSassinator The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
No description provided.