Fix `alloc_bytes` of ReadLimiter #5852

JinheLin · 2022-09-13T02:27:02Z

What problem does this PR solve?

Issue Number: close #5801

Problem Summary:

The calculation of alloc_bytes is not right when the burst read traffic exceeds the limit and results in the I/O limiter-tuner not working well.
After the burst read traffic exceeds the limit, the available_balance is negative (overdrawn), refillAndAlloc will refresh it periodically. However, ReadLimiter may not be called for a long time and refillAndAlloc will not be called either.

What is changed and how it works?

Before this PR:

Include this PR:

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

ti-chi-bot · 2022-09-13T02:27:04Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Lloyd-Pottiger
flowbehappy

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.

JinheLin · 2022-09-13T03:11:50Z

/run-all-tests

flowbehappy · 2022-09-13T04:23:04Z

dbms/src/Encryption/RateLimiter.cpp

@@ -191,6 +210,7 @@ void WriteLimiter::request(Int64 bytes)
 }
 }
 }
+ LOG_FMT_DEBUG(log, "pending_us {} wait_times {} pending_count {} rate_limit_per_sec {}", sw_pending.elapsed() / 1000, wait_times, req_queue.size(), refill_balance_per_period * 1000 / refill_period_ms);


This is too chatty. Use trace please.

And perhaps report to grafana if neccesary?

dbms/src/Encryption/RateLimiter.cpp

JinheLin · 2022-09-13T06:18:25Z

/run-all-tests

JinheLin · 2022-09-13T06:40:45Z

/run-all-tests

JinheLin · 2022-09-13T09:59:04Z

/run-all-tests

sre-bot · 2022-09-13T10:33:01Z

Coverage for changed files

Filename                                    Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Encryption/RateLimiter.cpp                      629               292    53.58%          46                13    71.74%         713               298    58.20%         296               136    54.05%
Encryption/RateLimiter.h                         70                 3    95.71%          27                 3    88.89%         102                23    77.45%          34                 6    82.35%
Encryption/tests/gtest_rate_limiter.cpp         313                61    80.51%          41                 0   100.00%         714                20    97.20%          56                10    82.14%
TestUtils/MockReadLimiter.h                       2                 0   100.00%           2                 0   100.00%           6                 0   100.00%           0                 0         -
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                          1014               356    64.89%         116                16    86.21%        1535               341    77.79%         386               152    60.62%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18669      8197             56.09%    215994  84401        60.92%

full coverage report (for internal network access only)

dbms/src/Encryption/RateLimiter.cpp

JinheLin · 2022-09-16T09:06:50Z

/merge

ti-chi-bot · 2022-09-16T09:06:51Z

@JinheLin: 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-09-16T09:06:53Z

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

Commit hash: fc1dbbd

sre-bot · 2022-09-16T09:33:44Z

Coverage for changed files

Filename                                    Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Encryption/RateLimiter.cpp                      623               288    53.77%          45                13    71.11%         698               290    58.45%         286               130    54.55%
Encryption/RateLimiter.h                         70                 3    95.71%          27                 3    88.89%         102                23    77.45%          34                 6    82.35%
Encryption/tests/gtest_rate_limiter.cpp         313                61    80.51%          41                 0   100.00%         714                20    97.20%          56                10    82.14%
TestUtils/MockReadLimiter.h                       2                 0   100.00%           2                 0   100.00%           6                 0   100.00%           0                 0         -
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                          1008               352    65.08%         115                16    86.09%        1520               333    78.09%         376               146    61.17%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18819      8124             56.83%    218562  83353        61.86%

full coverage report (for internal network access only)

JinheLin · 2022-09-16T11:15:44Z

/merge

ti-chi-bot · 2022-09-16T11:15:45Z

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

sre-bot · 2022-09-16T11:18:20Z

Coverage for changed files

Filename                                    Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Encryption/RateLimiter.cpp                      623               288    53.77%          45                13    71.11%         698               290    58.45%         286               130    54.55%
Encryption/RateLimiter.h                         70                 3    95.71%          27                 3    88.89%         102                23    77.45%          34                 6    82.35%
Encryption/tests/gtest_rate_limiter.cpp         313                61    80.51%          41                 0   100.00%         714                20    97.20%          56                10    82.14%
TestUtils/MockReadLimiter.h                       2                 0   100.00%           2                 0   100.00%           6                 0   100.00%           0                 0         -
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                          1008               352    65.08%         115                16    86.09%        1520               333    78.09%         376               146    61.17%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18821      8118             56.87%    218646  83328        61.89%

full coverage report (for internal network access only)

sre-bot · 2022-09-16T11:29:30Z

Coverage for changed files

Filename                                    Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Encryption/RateLimiter.cpp                      623               288    53.77%          45                13    71.11%         698               290    58.45%         286               131    54.20%
Encryption/RateLimiter.h                         70                 3    95.71%          27                 3    88.89%         102                23    77.45%          34                 6    82.35%
Encryption/tests/gtest_rate_limiter.cpp         313                60    80.83%          41                 0   100.00%         714                20    97.20%          56                10    82.14%
TestUtils/MockReadLimiter.h                       2                 0   100.00%           2                 0   100.00%           6                 0   100.00%           0                 0         -
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                          1008               351    65.18%         115                16    86.09%        1520               333    78.09%         376               147    60.90%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18821      8117             56.87%    218646  83404        61.85%

full coverage report (for internal network access only)

ti-chi-bot · 2022-09-16T11:29:46Z

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

At the same time I will also trigger all tests for you:

/run-all-tests

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.

sre-bot · 2022-09-16T12:01:38Z

Coverage for changed files

Filename                                    Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Encryption/RateLimiter.cpp                      623               288    53.77%          45                13    71.11%         698               290    58.45%         286               131    54.20%
Encryption/RateLimiter.h                         70                 3    95.71%          27                 3    88.89%         102                23    77.45%          34                 6    82.35%
Encryption/tests/gtest_rate_limiter.cpp         313                61    80.51%          41                 0   100.00%         714                20    97.20%          56                10    82.14%
TestUtils/MockReadLimiter.h                       2                 0   100.00%           2                 0   100.00%           6                 0   100.00%           0                 0         -
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                          1008               352    65.08%         115                16    86.09%        1520               333    78.09%         376               147    60.90%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18837      8105             56.97%    218800  83332        61.91%

full coverage report (for internal network access only)

ti-chi-bot · 2022-09-16T12:03:37Z

In response to a cherrypick label: new pull request created: #5930.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

close #5801

flowbehappy reviewed Sep 13, 2022

View reviewed changes

dbms/src/Encryption/RateLimiter.cpp Show resolved Hide resolved

JinheLin force-pushed the fix_read_limiter branch from dd64e76 to c0b994e Compare September 16, 2022 00:25

JinheLin changed the title ~~WIP: Fix alloc_bytes of ReadLimiter~~ Fix alloc_bytes of ReadLimiter Sep 16, 2022

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 16, 2022

JinheLin requested review from Lloyd-Pottiger, JaySon-Huang and lidezhu September 16, 2022 03:14

Lloyd-Pottiger reviewed Sep 16, 2022

View reviewed changes

dbms/src/Encryption/RateLimiter.cpp Outdated Show resolved Hide resolved

flowbehappy approved these changes Sep 16, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 16, 2022

JinheLin added 7 commits September 16, 2022 15:49

Fix refillAndAlloc and alloc_bytes of ReadLimiter

06c06b1

refine log

723a217

Fix metrics and unit tests.

b8fffbe

format

1c7ebeb

Fix unittests.

ffeab9b

use magic_enum

2cf4f33

log

fc1dbbd

JinheLin force-pushed the fix_read_limiter branch from c0b994e to fc1dbbd Compare September 16, 2022 08:06

Lloyd-Pottiger approved these changes Sep 16, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 16, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 16, 2022

Merge branch 'master' into fix_read_limiter

a2d7702

ti-chi-bot added 2 commits September 16, 2022 18:03

Merge branch 'master' into fix_read_limiter

66ec7e4

Merge branch 'master' into fix_read_limiter

f470a9e

Merge branch 'master' into fix_read_limiter

65b3ce3

ti-chi-bot merged commit b232bcd into pingcap:master Sep 16, 2022

ti-chi-bot mentioned this pull request Sep 16, 2022

Fix alloc_bytes of ReadLimiter (#5852) #5930

Merged

12 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request Sep 16, 2022

This is an automated cherry-pick of pingcap#5852

64c9b31

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot added a commit that referenced this pull request Oct 9, 2022

Fix alloc_bytes of ReadLimiter (#5852) (#5930)

7668a60

close #5801

JinheLin deleted the fix_read_limiter branch March 27, 2023 08:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `alloc_bytes` of ReadLimiter #5852

Fix `alloc_bytes` of ReadLimiter #5852

JinheLin commented Sep 13, 2022 •

edited

Loading

ti-chi-bot commented Sep 13, 2022 •

edited

Loading

JinheLin commented Sep 13, 2022

flowbehappy Sep 13, 2022

flowbehappy Sep 13, 2022

JinheLin commented Sep 13, 2022

JinheLin commented Sep 13, 2022

JinheLin commented Sep 13, 2022

sre-bot commented Sep 13, 2022

JinheLin commented Sep 16, 2022

ti-chi-bot commented Sep 16, 2022

ti-chi-bot commented Sep 16, 2022

sre-bot commented Sep 16, 2022

JinheLin commented Sep 16, 2022

ti-chi-bot commented Sep 16, 2022

sre-bot commented Sep 16, 2022

sre-bot commented Sep 16, 2022

ti-chi-bot commented Sep 16, 2022

sre-bot commented Sep 16, 2022

ti-chi-bot commented Sep 16, 2022

Fix alloc_bytes of ReadLimiter #5852

Fix alloc_bytes of ReadLimiter #5852

Conversation

JinheLin commented Sep 13, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Sep 13, 2022 • edited Loading

JinheLin commented Sep 13, 2022

flowbehappy Sep 13, 2022

Choose a reason for hiding this comment

flowbehappy Sep 13, 2022

Choose a reason for hiding this comment

JinheLin commented Sep 13, 2022

JinheLin commented Sep 13, 2022

JinheLin commented Sep 13, 2022

sre-bot commented Sep 13, 2022

Coverage for changed files

Coverage summary

JinheLin commented Sep 16, 2022

ti-chi-bot commented Sep 16, 2022

ti-chi-bot commented Sep 16, 2022

sre-bot commented Sep 16, 2022

Coverage for changed files

Coverage summary

JinheLin commented Sep 16, 2022

ti-chi-bot commented Sep 16, 2022

sre-bot commented Sep 16, 2022

Coverage for changed files

Coverage summary

sre-bot commented Sep 16, 2022

Coverage for changed files

Coverage summary

ti-chi-bot commented Sep 16, 2022

sre-bot commented Sep 16, 2022

Coverage for changed files

Coverage summary

ti-chi-bot commented Sep 16, 2022

Fix `alloc_bytes` of ReadLimiter #5852

Fix `alloc_bytes` of ReadLimiter #5852

JinheLin commented Sep 13, 2022 •

edited

Loading

ti-chi-bot commented Sep 13, 2022 •

edited

Loading