make read quota limiter work for coprocessor as well #13257

tonyxuqqi · 2022-08-11T06:10:56Z

Signed-off-by: qi.xu tonxuqi@outlook.com

What is changed and how it works?

Issue Number: Close #13256

What's Changed:

The coprocessor's read bytes are not calculated by foreground quota limiter.

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
Need to cherry-pick to the release branch

Check List

Tests

Unit test
Manual test

Side effects

None

Release note

None

Signed-off-by: qi.xu <tonxuqi@outlook.com>

ti-chi-bot · 2022-08-11T06:10:57Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

5kbpers
ethercflow

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.

tonyxuqqi · 2022-08-11T06:11:43Z

/cc BornChanger

ti-chi-bot · 2022-08-11T06:11:45Z

@tonyxuqqi: GitHub didn't allow me to request PR reviews from the following users: BornChanger.

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

In response to this:

/cc BornChanger

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.

BornChanger · 2022-08-11T08:59:26Z

components/tidb_query_executors/src/runner.rs

@@ -471,6 +471,9 @@ impl<SS: 'static> BatchExecutorsRunner<SS> {
                    &mut ctx,
                )?
            };
+            if chunk.has_rows_data() {


Though I prefer to use following approach. ForwardScanner maintains the statistics (via the Cursor interface), but it's lost before returning to internal_handle_request() in the test. This patch is ok to me at present.

note here the limiter is foreground limiter---that is to limit actual response size back to TiDB, not the IO limiter.

BornChanger · 2022-08-11T09:00:15Z

tests/integrations/coprocessor/test_select.rs

@@ -76,6 +77,7 @@ fn test_select() {
        let result_encoded = datum::encode_value(&mut EvalContext::default(), &row).unwrap();
        assert_eq!(result_encoded, &*expected_encoded);
    }
+    assert!(limiter.total_read_bytes_consumed(true) > 0); // the consume_sample is called due to read bytes quota


why not give a solid number?

OK, looks that it's doable.

Signed-off-by: qi.xu <tonxuqi@outlook.com>

…fix_limiter

Signed-off-by: qi.xu <tonxuqi@outlook.com>

ti-chi-bot · 2022-08-17T06:56:49Z

@BornChanger: 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.

ethercflow

LGTM

tonyxuqqi · 2022-08-22T20:50:18Z

/test

5kbpers · 2022-08-23T01:53:01Z

/merge

ti-chi-bot · 2022-08-23T01:53:02Z

@5kbpers: 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 · 2022-08-23T01:53:04Z

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

Commit hash: 848ec5d

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2022-08-23T01:54:47Z

cherry pick to release-6.0 in PR #13323

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2022-08-23T01:55:14Z

cherry pick to release-6.1 in PR #13324

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2022-08-23T01:55:40Z

cherry pick to release-6.2 in PR #13325

ref #13257, close #13301 Signed-off-by: BornChanger <dawn_catcher@126.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>

ref tikv#13257, close tikv#13301 Signed-off-by: BornChanger <dawn_catcher@126.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io> Signed-off-by: TonsnakeLin <lpbgytong@163.com>

close tikv#13256 The coprocessor's read bytes are not calculated by foreground quota limiter. Signed-off-by: qi.xu <tonxuqi@outlook.com> Co-authored-by: qi.xu <tonxuqi@outlook.com> Co-authored-by: 5kbpers <hustmh@gmail.com> Signed-off-by: fengou1 <feng.ou@pingcap.com>

ref tikv#13257, close tikv#13301 Signed-off-by: BornChanger <dawn_catcher@126.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io> Signed-off-by: fengou1 <feng.ou@pingcap.com>

close #13256, ref #13257 The coprocessor's read bytes are not calculated by foreground quota limiter. Signed-off-by: qi.xu <tonxuqi@outlook.com> Signed-off-by: tonyxuqqi <tonyxuqi@outlook.com> Co-authored-by: qi.xu <tonxuqi@outlook.com> Co-authored-by: tonyxuqqi <tonyxuqi@outlook.com>

make read quota limiter work for coprocessor as well

030887a

Signed-off-by: qi.xu <tonxuqi@outlook.com>

ti-chi-bot added release-note-none do-not-merge/needs-triage-completed size/M labels Aug 11, 2022

Merge branch 'master' into fix_limiter

2f9dbf6

tonyxuqqi requested review from 5kbpers and ethercflow August 11, 2022 06:13

BornChanger reviewed Aug 11, 2022

View reviewed changes

qi.xu and others added 6 commits August 12, 2022 01:18

fix lint

16d0266

Signed-off-by: qi.xu <tonxuqi@outlook.com>

Merge branch 'fix_limiter' of https://github.com/tonyxuqqi/tikv into …

5a9ee8f

…fix_limiter

Merge branch 'master' into fix_limiter

bfbc31e

address code review comments

6763e23

Signed-off-by: qi.xu <tonxuqi@outlook.com>

fix lint

c1ad74e

Signed-off-by: qi.xu <tonxuqi@outlook.com>

fix lint

21bbeea

Signed-off-by: qi.xu <tonxuqi@outlook.com>

BornChanger approved these changes Aug 17, 2022

View reviewed changes

ethercflow approved these changes Aug 17, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Status: PR - There is already 1 approval label Aug 17, 2022

5kbpers approved these changes Aug 17, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Status: PR - There are already 2 approvals and removed status/LGT1 Status: PR - There is already 1 approval labels Aug 17, 2022

Merge branch 'master' into fix_limiter

db8de3d

ti-chi-bot added needs-cherry-pick-release-6.0 needs-cherry-pick-release-6.1 needs-cherry-pick-release-6.2 and removed do-not-merge/needs-triage-completed labels Aug 22, 2022

Merge branch 'master' into fix_limiter

848ec5d

ti-chi-bot added the status/can-merge Status: Can merge to base branch label Aug 23, 2022

ti-chi-bot merged commit 252b442 into tikv:master Aug 23, 2022

ti-chi-bot added this to the Pool milestone Aug 23, 2022

ti-srebot mentioned this pull request Aug 23, 2022

make read quota limiter work for coprocessor as well (#13257) #13323

Closed

ti-srebot pushed a commit to ti-srebot/tikv that referenced this pull request Aug 23, 2022

cherry pick tikv#13257 to release-6.0

d414187

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Aug 23, 2022

make read quota limiter work for coprocessor as well (#13257) #13324

Merged

ti-srebot pushed a commit to ti-srebot/tikv that referenced this pull request Aug 23, 2022

cherry pick tikv#13257 to release-6.1

c4547f2

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot pushed a commit to ti-srebot/tikv that referenced this pull request Aug 23, 2022

cherry pick tikv#13257 to release-6.2

2653746

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Aug 23, 2022

make read quota limiter work for coprocessor as well (#13257) #13325

Closed

ti-chi-bot added a commit that referenced this pull request Aug 25, 2022

*: support read quota limit for analyze (#13302)

0030aeb

ref #13257, close #13301 Signed-off-by: BornChanger <dawn_catcher@126.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make read quota limiter work for coprocessor as well #13257

make read quota limiter work for coprocessor as well #13257

tonyxuqqi commented Aug 11, 2022

ti-chi-bot commented Aug 11, 2022 •

edited

tonyxuqqi commented Aug 11, 2022

ti-chi-bot commented Aug 11, 2022

BornChanger Aug 11, 2022

tonyxuqqi Aug 11, 2022 •

edited

BornChanger Aug 11, 2022

tonyxuqqi Aug 11, 2022 •

edited

ti-chi-bot commented Aug 17, 2022

ethercflow left a comment

tonyxuqqi commented Aug 22, 2022

5kbpers commented Aug 23, 2022

ti-chi-bot commented Aug 23, 2022

ti-chi-bot commented Aug 23, 2022

ti-srebot commented Aug 23, 2022

ti-srebot commented Aug 23, 2022

ti-srebot commented Aug 23, 2022

make read quota limiter work for coprocessor as well #13257

make read quota limiter work for coprocessor as well #13257

Conversation

tonyxuqqi commented Aug 11, 2022

What is changed and how it works?

Related changes

Check List

Release note

ti-chi-bot commented Aug 11, 2022 • edited

tonyxuqqi commented Aug 11, 2022

ti-chi-bot commented Aug 11, 2022

BornChanger Aug 11, 2022

Choose a reason for hiding this comment

tonyxuqqi Aug 11, 2022 • edited

Choose a reason for hiding this comment

BornChanger Aug 11, 2022

Choose a reason for hiding this comment

tonyxuqqi Aug 11, 2022 • edited

Choose a reason for hiding this comment

ti-chi-bot commented Aug 17, 2022

ethercflow left a comment

Choose a reason for hiding this comment

tonyxuqqi commented Aug 22, 2022

5kbpers commented Aug 23, 2022

ti-chi-bot commented Aug 23, 2022

ti-chi-bot commented Aug 23, 2022

ti-srebot commented Aug 23, 2022

ti-srebot commented Aug 23, 2022

ti-srebot commented Aug 23, 2022

ti-chi-bot commented Aug 11, 2022 •

edited

tonyxuqqi Aug 11, 2022 •

edited

tonyxuqqi Aug 11, 2022 •

edited