Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: improve RUNTIME_CHECK and RUNTIME_ASSERT #5434

Merged
merged 13 commits into from
Jul 27, 2022

Conversation

fuzhe1989
Copy link
Contributor

@fuzhe1989 fuzhe1989 commented Jul 21, 2022

Signed-off-by: fuzhe1989 fuzhe1989@gmail.com

What problem does this PR solve?

Issue Number: close #5444

Problem Summary:

What is changed and how it works?

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

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

None

Signed-off-by: fuzhe1989 <fuzhe1989@gmail.com>
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jul 21, 2022

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • JaySon-Huang
  • gengliqi

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.

@ti-chi-bot ti-chi-bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 21, 2022
@fuzhe1989
Copy link
Contributor Author

/cc @SeaRise @SchrodingerZhu

@ti-chi-bot ti-chi-bot requested a review from SeaRise July 21, 2022 07:48
@ti-chi-bot
Copy link
Member

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

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

In response to this:

/cc @SeaRise @SchrodingerZhu

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.

@fuzhe1989
Copy link
Contributor Author

/cc @gengliqi

@ti-chi-bot ti-chi-bot requested a review from gengliqi July 21, 2022 07:56
@JaySon-Huang
Copy link
Contributor

/run-all-tests

@fuzhe1989
Copy link
Contributor Author

/run-all-tests

@sre-bot
Copy link
Collaborator

sre-bot commented Jul 22, 2022

Coverage for changed files

Filename                                           Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Common/Exception.cpp                                   103                90    12.62%          14                11    21.43%         164               121    26.22%          40                37     7.50%
Common/Exception.h                                      37                30    18.92%          26                19    26.92%          88                81     7.95%           8                 8     0.00%
Common/Logger.h                                         20                 4    80.00%          18                 4    77.78%          63                18    71.43%           2                 0   100.00%
Flash/Coprocessor/DAGQueryBlockInterpreter.cpp         225                53    76.44%          33                 5    84.85%         601               134    77.70%         152                44    71.05%
Flash/Mpp/ExchangeReceiver.cpp                         409               409     0.00%          35                35     0.00%         596               596     0.00%         240               240     0.00%
TestUtils/FunctionTestUtils.h                          127                10    92.13%          38                 0   100.00%         281                 5    98.22%          52                 8    84.62%
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                  921               596    35.29%         164                74    54.88%        1793               955    46.74%         494               337    31.78%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18788      9457             49.66%    214059  95551        55.36%

full coverage report (for internal network access only)

@SeaRise
Copy link
Contributor

SeaRise commented Jul 22, 2022

/run-integration-test

@JaySon-Huang
Copy link
Contributor

/run-unit-test

@sre-bot
Copy link
Collaborator

sre-bot commented Jul 22, 2022

Coverage for changed files

Filename                                           Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Common/Exception.cpp                                   103                90    12.62%          14                11    21.43%         164               121    26.22%          40                37     7.50%
Common/Exception.h                                      37                30    18.92%          26                19    26.92%          88                81     7.95%           8                 8     0.00%
Common/Logger.h                                         20                 4    80.00%          18                 4    77.78%          63                18    71.43%           2                 0   100.00%
Flash/Coprocessor/DAGQueryBlockInterpreter.cpp         225                53    76.44%          33                 5    84.85%         601               134    77.70%         152                44    71.05%
Flash/Mpp/ExchangeReceiver.cpp                         409               409     0.00%          35                35     0.00%         596               596     0.00%         240               240     0.00%
TestUtils/FunctionTestUtils.h                          127                10    92.13%          38                 0   100.00%         281                 5    98.22%          52                 8    84.62%
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                  921               596    35.29%         164                74    54.88%        1793               955    46.74%         494               337    31.78%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18807      9461             49.69%    214247  95566        55.39%

full coverage report (for internal network access only)

@fuzhe1989
Copy link
Contributor Author

any update? @JaySon-Huang @SeaRise @gengliqi

@fuzhe1989
Copy link
Contributor Author

/run-all-tests

@sre-bot
Copy link
Collaborator

sre-bot commented Jul 25, 2022

Coverage for changed files

Filename                                           Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Common/Exception.cpp                                   103                90    12.62%          14                11    21.43%         165               122    26.06%          40                37     7.50%
Common/Exception.h                                      31                24    22.58%          24                17    29.17%          68                61    10.29%           4                 4     0.00%
Common/FailPoint.cpp                                   587               172    70.70%           8                 2    75.00%          84                32    61.90%         200                99    50.50%
Common/Logger.h                                         20                 4    80.00%          18                 4    77.78%          63                18    71.43%           2                 0   100.00%
Flash/Coprocessor/DAGQueryBlockInterpreter.cpp         225                53    76.44%          33                 5    84.85%         601               134    77.70%         152                44    71.05%
Flash/Mpp/ExchangeReceiver.cpp                         409               409     0.00%          35                35     0.00%         596               596     0.00%         240               240     0.00%
TestUtils/FunctionTestUtils.h                          127                10    92.13%          38                 0   100.00%         281                 5    98.22%          52                 8    84.62%

Files which contain no functions:
Common/FailPoint.h                                       0                 0         -           0                 0         -           0                 0         -           0                 0         -
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                 1502               762    49.27%         170                74    56.47%        1858               968    47.90%         690               432    37.39%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18817      9460             49.73%    214377  95579        55.42%

full coverage report (for internal network access only)

}

template <typename... Args>
inline void log(const char * filename, int lineno, const char * condition, const LoggerPtr & logger, const char * fmt_str, Args &&... args)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about removing const char * fmt_str?
If so, inline void log(const char * filename, int lineno, const char * condition, const LoggerPtr & logger) can be removed too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}

template <typename... Args>
inline void log(const char * filename, int lineno, const char * condition, const char * fmt_str, Args &&... args)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this fmt_str is used for overloadding from const LoggerPtr & or this version will always be chosen even when there's a const LoggerPtr &.

Signed-off-by: fuzhe1989 <fuzhe1989@gmail.com>
Copy link
Contributor

@JaySon-Huang JaySon-Huang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 26, 2022
@fuzhe1989
Copy link
Contributor Author

/run-all-tests

@sre-bot
Copy link
Collaborator

sre-bot commented Jul 26, 2022

Coverage for changed files

Filename                                           Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Common/Exception.cpp                                   103                90    12.62%          14                11    21.43%         165               122    26.06%          40                37     7.50%
Common/Exception.h                                      28                21    25.00%          23                16    30.43%          60                53    11.67%           2                 2     0.00%
Common/FailPoint.cpp                                   587               172    70.70%           8                 2    75.00%          84                32    61.90%         200                99    50.50%
Common/Logger.h                                         20                 4    80.00%          18                 4    77.78%          63                18    71.43%           2                 0   100.00%
Common/TiFlashSecurity.h                                19                 4    78.95%           6                 4    33.33%         110                93    15.45%          14                 5    64.29%
Encryption/RateLimiter.cpp                             536               252    52.99%          46                13    71.74%         689               272    60.52%         292               138    52.74%
Encryption/RateLimiter.h                                68                 2    97.06%          28                 2    92.86%         107                20    81.31%          32                 6    81.25%
Flash/Coprocessor/DAGQueryBlockInterpreter.cpp         225                53    76.44%          33                 5    84.85%         601               134    77.70%         152                44    71.05%
Flash/Mpp/ExchangeReceiver.cpp                         409               409     0.00%          35                35     0.00%         596               596     0.00%         240               240     0.00%
Server/RaftConfigParser.cpp                             70                70     0.00%           4                 4     0.00%          85                85     0.00%          40                40     0.00%
Server/RaftConfigParser.h                                1                 0   100.00%           1                 0   100.00%           1                 0   100.00%           0                 0         -
Server/StorageConfigParser.cpp                         407                89    78.13%          23                 1    95.65%         425                23    94.59%         236                85    63.98%
Server/StorageConfigParser.h                             2                 0   100.00%           2                 0   100.00%           2                 0   100.00%           0                 0         -
Server/UserConfigParser.cpp                             24                24     0.00%           3                 3     0.00%          35                35     0.00%          12                12     0.00%
Server/tests/gtest_storage_config.cpp                 3067              1055    65.60%          16                 2    87.50%         649                34    94.76%         936               562    39.96%
TestUtils/FunctionTestUtils.h                          127                10    92.13%          38                 0   100.00%         281                 5    98.22%          52                 8    84.62%

Files which contain no functions:
Common/FailPoint.h                                       0                 0         -           0                 0         -           0                 0         -           0                 0         -
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                 5693              2255    60.39%         298               102    65.77%        3953              1522    61.50%        2250              1278    43.20%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18816      9456             49.74%    214373  95580        55.41%

full coverage report (for internal network access only)

@ti-chi-bot 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 Jul 26, 2022
@fuzhe1989
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

@fuzhe1989: 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
Copy link
Member

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

Commit hash: 709cd71

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 26, 2022
@sre-bot
Copy link
Collaborator

sre-bot commented Jul 26, 2022

Coverage for changed files

Filename                                           Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Common/Exception.cpp                                   103                90    12.62%          14                11    21.43%         165               122    26.06%          40                37     7.50%
Common/Exception.h                                      28                21    25.00%          23                16    30.43%          60                53    11.67%           2                 2     0.00%
Common/FailPoint.cpp                                   587               172    70.70%           8                 2    75.00%          84                32    61.90%         200                99    50.50%
Common/Logger.h                                         20                 4    80.00%          18                 4    77.78%          63                18    71.43%           2                 0   100.00%
Common/TiFlashSecurity.h                                19                 4    78.95%           6                 4    33.33%         110                93    15.45%          14                 5    64.29%
Encryption/RateLimiter.cpp                             536               252    52.99%          46                13    71.74%         689               272    60.52%         292               138    52.74%
Encryption/RateLimiter.h                                68                 2    97.06%          28                 2    92.86%         107                20    81.31%          32                 6    81.25%
Flash/Coprocessor/DAGQueryBlockInterpreter.cpp         225                53    76.44%          33                 5    84.85%         601               134    77.70%         152                44    71.05%
Flash/Mpp/ExchangeReceiver.cpp                         409               409     0.00%          35                35     0.00%         596               596     0.00%         240               240     0.00%
Server/RaftConfigParser.cpp                             70                70     0.00%           4                 4     0.00%          85                85     0.00%          40                40     0.00%
Server/RaftConfigParser.h                                1                 0   100.00%           1                 0   100.00%           1                 0   100.00%           0                 0         -
Server/StorageConfigParser.cpp                         407                89    78.13%          23                 1    95.65%         425                23    94.59%         236                85    63.98%
Server/StorageConfigParser.h                             2                 0   100.00%           2                 0   100.00%           2                 0   100.00%           0                 0         -
Server/UserConfigParser.cpp                             24                24     0.00%           3                 3     0.00%          35                35     0.00%          12                12     0.00%
Server/tests/gtest_storage_config.cpp                 3067              1055    65.60%          16                 2    87.50%         649                34    94.76%         936               562    39.96%
TestUtils/FunctionTestUtils.h                          127                10    92.13%          38                 0   100.00%         281                 5    98.22%          52                 8    84.62%

Files which contain no functions:
Common/FailPoint.h                                       0                 0         -           0                 0         -           0                 0         -           0                 0         -
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                 5693              2255    60.39%         298               102    65.77%        3953              1522    61.50%        2250              1278    43.20%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18816      9456             49.74%    214373  95556        55.43%

full coverage report (for internal network access only)

@fuzhe1989
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

@fuzhe1989: 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
Copy link
Collaborator

sre-bot commented Jul 26, 2022

Coverage for changed files

Filename                                           Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Common/Exception.cpp                                   103                90    12.62%          14                11    21.43%         165               122    26.06%          40                37     7.50%
Common/Exception.h                                      28                21    25.00%          23                16    30.43%          60                53    11.67%           2                 2     0.00%
Common/FailPoint.cpp                                   587               172    70.70%           8                 2    75.00%          84                32    61.90%         200                99    50.50%
Common/Logger.h                                         20                 4    80.00%          18                 4    77.78%          63                18    71.43%           2                 0   100.00%
Common/TiFlashSecurity.h                                19                 4    78.95%           6                 4    33.33%         110                93    15.45%          14                 5    64.29%
Encryption/RateLimiter.cpp                             536               252    52.99%          46                13    71.74%         689               272    60.52%         292               138    52.74%
Encryption/RateLimiter.h                                68                 2    97.06%          28                 2    92.86%         107                20    81.31%          32                 6    81.25%
Flash/Coprocessor/DAGQueryBlockInterpreter.cpp         225                53    76.44%          33                 5    84.85%         601               134    77.70%         152                44    71.05%
Flash/Mpp/ExchangeReceiver.cpp                         409               409     0.00%          35                35     0.00%         596               596     0.00%         240               240     0.00%
Server/RaftConfigParser.cpp                             70                70     0.00%           4                 4     0.00%          85                85     0.00%          40                40     0.00%
Server/RaftConfigParser.h                                1                 0   100.00%           1                 0   100.00%           1                 0   100.00%           0                 0         -
Server/StorageConfigParser.cpp                         407                89    78.13%          23                 1    95.65%         425                23    94.59%         236                85    63.98%
Server/StorageConfigParser.h                             2                 0   100.00%           2                 0   100.00%           2                 0   100.00%           0                 0         -
Server/UserConfigParser.cpp                             24                24     0.00%           3                 3     0.00%          35                35     0.00%          12                12     0.00%
Server/tests/gtest_storage_config.cpp                 3067              1055    65.60%          16                 2    87.50%         649                34    94.76%         936               562    39.96%
TestUtils/FunctionTestUtils.h                          127                10    92.13%          38                 0   100.00%         281                 5    98.22%          52                 8    84.62%

Files which contain no functions:
Common/FailPoint.h                                       0                 0         -           0                 0         -           0                 0         -           0                 0         -
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                 5693              2255    60.39%         298               102    65.77%        3953              1522    61.50%        2250              1278    43.20%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18832      9438             49.88%    214733  95396        55.57%

full coverage report (for internal network access only)

@sre-bot
Copy link
Collaborator

sre-bot commented Jul 27, 2022

Coverage for changed files

Filename                                           Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Common/Exception.cpp                                   103                90    12.62%          14                11    21.43%         165               122    26.06%          40                37     7.50%
Common/Exception.h                                      28                21    25.00%          23                16    30.43%          60                53    11.67%           2                 2     0.00%
Common/FailPoint.cpp                                   587               172    70.70%           8                 2    75.00%          84                32    61.90%         200                99    50.50%
Common/Logger.h                                         20                 4    80.00%          18                 4    77.78%          63                18    71.43%           2                 0   100.00%
Common/TiFlashSecurity.h                                19                 4    78.95%           6                 4    33.33%         110                93    15.45%          14                 5    64.29%
Encryption/RateLimiter.cpp                             536               252    52.99%          46                13    71.74%         689               272    60.52%         292               138    52.74%
Encryption/RateLimiter.h                                68                 2    97.06%          28                 2    92.86%         107                20    81.31%          32                 6    81.25%
Flash/Coprocessor/DAGQueryBlockInterpreter.cpp         225                53    76.44%          33                 5    84.85%         601               134    77.70%         152                44    71.05%
Flash/Mpp/ExchangeReceiver.cpp                         409               409     0.00%          35                35     0.00%         596               596     0.00%         240               240     0.00%
Server/RaftConfigParser.cpp                             70                70     0.00%           4                 4     0.00%          85                85     0.00%          40                40     0.00%
Server/RaftConfigParser.h                                1                 0   100.00%           1                 0   100.00%           1                 0   100.00%           0                 0         -
Server/StorageConfigParser.cpp                         407                89    78.13%          23                 1    95.65%         425                23    94.59%         236                85    63.98%
Server/StorageConfigParser.h                             2                 0   100.00%           2                 0   100.00%           2                 0   100.00%           0                 0         -
Server/UserConfigParser.cpp                             24                24     0.00%           3                 3     0.00%          35                35     0.00%          12                12     0.00%
Server/tests/gtest_storage_config.cpp                 3067              1055    65.60%          16                 2    87.50%         649                34    94.76%         936               562    39.96%
TestUtils/FunctionTestUtils.h                          127                10    92.13%          38                 0   100.00%         281                 5    98.22%          52                 8    84.62%

Files which contain no functions:
Common/FailPoint.h                                       0                 0         -           0                 0         -           0                 0         -           0                 0         -
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                 5693              2255    60.39%         298               102    65.77%        3953              1522    61.50%        2250              1278    43.20%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18832      9439             49.88%    214733  95399        55.57%

full coverage report (for internal network access only)

@ti-chi-bot ti-chi-bot merged commit e5f0eb4 into pingcap:master Jul 27, 2022
@fuzhe1989 fuzhe1989 deleted the fuzhe/runtime_check branch July 27, 2022 03:30
@breezewish breezewish mentioned this pull request Aug 12, 2022
12 tasks
JaySon-Huang pushed a commit to ti-chi-bot/tiflash that referenced this pull request Aug 31, 2022
close pingcap#5444

 Conflicts:
	dbms/src/Common/FailPoint.cpp
	dbms/src/Common/FailPoint.h
	dbms/src/Flash/Coprocessor/DAGQueryBlockInterpreter.cpp
	dbms/src/Flash/Mpp/ExchangeReceiver.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better RUNTIME_CHECK and RUNTIME_ASSERT
6 participants