*: add skip store limit option for scatter region #6593

rleungx · 2023-06-13T08:11:44Z

What problem does this PR solve?

Issue Number: Close #6592

What is changed and how does it work?

This PR supports light peer for removing peer step. If we are doing scatter operation, there is an option to skip the store limit check.

Check List

Tests

Unit test

Release note

None.

ti-chi-bot · 2023-06-13T08:11:47Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JmPotato
bufferflies

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

codecov · 2023-06-13T08:34:11Z

Codecov Report

Merging #6593 (ab06ff7) into master (4db1735) will decrease coverage by 0.13%.
The diff coverage is 90.32%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6593      +/-   ##
==========================================
- Coverage   74.17%   74.05%   -0.13%     
==========================================
  Files         415      415              
  Lines       43920    43926       +6     
==========================================
- Hits        32579    32529      -50     
- Misses       8433     8468      +35     
- Partials     2908     2929      +21

Flag	Coverage Δ
unittests	`74.05% <90.32%> (-0.13%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

nolouch

I wonder if the lightning scatter non-empty region, it's may cause tikv be busy and even influence the cluster if during the online import.

3pointer · 2023-06-16T08:38:27Z

I wonder if the lightning scatter non-empty region, it's may cause tikv be busy and even influence the cluster if during the online import.

According to my knowledge online import should only scatter the empty region. like restore a new table without any traffic. @lance6716 can you confirm this?

lance6716 · 2023-06-16T08:51:31Z

I wonder if the lightning scatter non-empty region, it's may cause tikv be busy and even influence the cluster if during the online import.

According to my knowledge online import should only scatter the empty region. like restore a new table without any traffic. @lance6716 can you confirm this?

@nolouch one case is for parallel import (multiple lightning instances are running), lightning's progress is not synchronized to it will scatter non-empty region because other lightning instance has started ingestion.

3pointer · 2023-06-16T08:54:25Z

I wonder if the lightning scatter non-empty region, it's may cause tikv be busy and even influence the cluster if during the online import.

According to my knowledge online import should only scatter the empty region. like restore a new table without any traffic. @lance6716 can you confirm this?

@nolouch one case is for parallel import (multiple lightning instances are running), lightning's progress is not synchronized to it will scatter non-empty region because other lightning instance has started ingestion.

but in this case cluster cannot serve traffic right? @lance6716

nolouch · 2023-06-26T12:17:07Z

Even the cluster do not serve, too many scatters also may let tikv has many pending snapshot, may let tikv not be normal. like too many pending peers.

JmPotato · 2023-07-03T02:25:27Z

@3pointer @lance6716 @nolouch Any updates on this PR?

3pointer · 2023-07-08T10:24:29Z

I think it should be caller's responsibility to set the store limit for different situation. so at least give an option of scatter region to let caller decide the limitation.

nolouch · 2023-07-10T02:48:09Z

I think it's ok to no limit in multi-rocks because the cost of generating a snapshot is relatively small. so add a option or directly judge in pd both ok to me.

pkg/schedule/operator/builder.go

ti-chi-bot · 2023-07-27T05:48:49Z

@rleungx: GitHub didn't allow me to request PR reviews from the following users: 3pointer.

Note that only tikv members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @3pointer

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.

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx · 2023-07-27T06:45:23Z

/hold cancel

nolouch

lgtm

ti-chi-bot · 2023-07-28T03:05:57Z

@3pointer: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In 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 ti-community-infra/tichi repository.

JmPotato · 2023-07-28T03:07:09Z

/merge

ti-chi-bot · 2023-07-28T03:07:11Z

@JmPotato: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

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 ti-community-infra/tichi repository.

ti-chi-bot · 2023-07-28T03:07:12Z

This pull request has been accepted and is ready to merge.

Commit hash: 56034ed

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx · 2023-07-28T03:28:17Z

/merge

ti-chi-bot · 2023-07-28T03:28:19Z

@rleungx: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

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 ti-community-infra/tichi repository.

ti-chi-bot · 2023-07-28T03:28:20Z

This pull request has been accepted and is ready to merge.

Commit hash: 91de394

ti-chi-bot · 2023-07-28T03:28:34Z

@rleungx: Your PR was out of date, I have automatically updated it for you.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

ti-chi-bot bot added the release-note-none Denotes a PR that doesn't merit a release note. label Jun 13, 2023

ti-chi-bot bot requested review from HunDunDM and lhy1024 June 13, 2023 08:11

ti-chi-bot bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 13, 2023

rleungx requested review from bufferflies and nolouch and removed request for lhy1024 and HunDunDM June 13, 2023 08:18

bufferflies approved these changes Jun 14, 2023

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 14, 2023

nolouch reviewed Jun 16, 2023

View reviewed changes

3pointer mentioned this pull request Jul 8, 2023

[Dynamic Regions] br: br restore to multirocks cluster causing tikv full space pingcap/tidb#44550

Closed

JmPotato reviewed Jul 10, 2023

View reviewed changes

pkg/schedule/operator/builder.go Outdated Show resolved Hide resolved

rleungx force-pushed the no-limit-scatter branch from fd8820a to fd24485 Compare July 17, 2023 04:17

JmPotato approved these changes Jul 17, 2023

View reviewed changes

ti-chi-bot bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 17, 2023

rleungx added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 17, 2023

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 26, 2023

fix test

2f2b16b

Signed-off-by: Ryan Leung <rleungx@gmail.com>

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 27, 2023

rleungx requested a review from nolouch July 27, 2023 07:00

rleungx force-pushed the no-limit-scatter branch from 54f9277 to 56034ed Compare July 27, 2023 07:44

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 27, 2023

rleungx changed the title ~~schedule: don't limit the scatter region~~ schedule: add skip store limit option for scatter region Jul 27, 2023

rleungx changed the title ~~schedule: add skip store limit option for scatter region~~ *: add skip store limit option for scatter region Jul 27, 2023

nolouch approved these changes Jul 27, 2023

View reviewed changes

3pointer approved these changes Jul 28, 2023

View reviewed changes

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 28, 2023

fix mod

be2ebc2

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx force-pushed the no-limit-scatter branch from 53b0f2e to be2ebc2 Compare July 28, 2023 03:09

ti-chi-bot bot removed the status/can-merge Indicates a PR has been approved by a committer. label Jul 28, 2023

rename

91de394

Signed-off-by: Ryan Leung <rleungx@gmail.com>

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 28, 2023

Merge branch 'master' into no-limit-scatter

ab06ff7

ti-chi-bot bot merged commit 31343e0 into tikv:master Jul 28, 2023
23 checks passed

rleungx deleted the no-limit-scatter branch July 28, 2023 03:39

rleungx mentioned this pull request Aug 11, 2023

skip store limit for emptry regions created by lightning #6951

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: add skip store limit option for scatter region #6593

*: add skip store limit option for scatter region #6593

rleungx commented Jun 13, 2023 •

edited

Loading

ti-chi-bot bot commented Jun 13, 2023 •

edited

Loading

codecov bot commented Jun 13, 2023 •

edited

Loading

nolouch left a comment

3pointer commented Jun 16, 2023

lance6716 commented Jun 16, 2023 •

edited

Loading

3pointer commented Jun 16, 2023

nolouch commented Jun 26, 2023

JmPotato commented Jul 3, 2023

3pointer commented Jul 8, 2023

nolouch commented Jul 10, 2023

ti-chi-bot bot commented Jul 27, 2023

rleungx commented Jul 27, 2023

nolouch left a comment

ti-chi-bot bot commented Jul 28, 2023

JmPotato commented Jul 28, 2023

ti-chi-bot bot commented Jul 28, 2023

ti-chi-bot bot commented Jul 28, 2023

rleungx commented Jul 28, 2023

ti-chi-bot bot commented Jul 28, 2023

ti-chi-bot bot commented Jul 28, 2023

ti-chi-bot bot commented Jul 28, 2023

*: add skip store limit option for scatter region #6593

*: add skip store limit option for scatter region #6593

Conversation

rleungx commented Jun 13, 2023 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Jun 13, 2023 • edited Loading

codecov bot commented Jun 13, 2023 • edited Loading

Codecov Report

nolouch left a comment

Choose a reason for hiding this comment

3pointer commented Jun 16, 2023

lance6716 commented Jun 16, 2023 • edited Loading

3pointer commented Jun 16, 2023

nolouch commented Jun 26, 2023

JmPotato commented Jul 3, 2023

3pointer commented Jul 8, 2023

nolouch commented Jul 10, 2023

ti-chi-bot bot commented Jul 27, 2023

rleungx commented Jul 27, 2023

nolouch left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jul 28, 2023

JmPotato commented Jul 28, 2023

ti-chi-bot bot commented Jul 28, 2023

ti-chi-bot bot commented Jul 28, 2023

rleungx commented Jul 28, 2023

ti-chi-bot bot commented Jul 28, 2023

ti-chi-bot bot commented Jul 28, 2023

ti-chi-bot bot commented Jul 28, 2023

rleungx commented Jun 13, 2023 •

edited

Loading

ti-chi-bot bot commented Jun 13, 2023 •

edited

Loading

codecov bot commented Jun 13, 2023 •

edited

Loading

lance6716 commented Jun 16, 2023 •

edited

Loading