Refine some interface & bug fixes for page stress testing by JaySon-Huang · Pull Request #3062 · pingcap/tiflash

JaySon-Huang · 2021-09-09T09:41:51Z

What problem does this PR solve?

Issue Number: close #xxx

Problem Summary:

What is changed and how it works?

Proposal: xxx

What's Changed:

Related changes

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

Check List

Tests

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

Side effects

Release note

None

Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>

ti-chi-bot · 2021-09-09T09:41:52Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

lidezhu

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.

Details

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

JaySon-Huang · 2021-09-09T09:43:07Z

dbms/src/Storages/Page/tests/stress/PSRunnable.cpp

-        /*Just for no warming*/
+        /*Just for no warning*/
    }
-    tracker.setDescription(description().c_str());


This is a use after free bug

Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>

jiaqizho

Anyway, I'm not sure if we already have a code style manual or are already developing it. So in many places, I subconsciously define it in the style of C.

Summarized the following points:

Check the explicit in class construction
Check const in function which in class
Do not use (int) to forced transfer type
Check the semicolon after the class member
Case-check-in name

And I am also kindly seeking your code style suggestion. Thanks a lot.

jiaqizho · 2021-09-10T02:01:19Z

dbms/src/Storages/Page/tests/stress/PSRunnable.cpp

+        /*Just for no warning*/
    }
-    tracker.setDescription(description().c_str());
+    auto peak = current_memory_tracker->getPeak();


I am changing this logic, mainly because subsequent changes need to support more metrics dump.

for now, PSMetricsDumper only supports CurrentMetrics::MemoryTracking, and i will add CurrentMetrics::PSMVCCSnapshotsList to check snapshots number. So we still need use PSMetricsDumper to get peak.

jiaqizho · 2021-09-10T02:07:41Z

dbms/src/Storages/Page/tests/stress/PSWorkload.cpp

+            auto workload = creator(options);
            LOG_INFO(StressEnv::logger, fmt::format("Start Running {} , {}", name, workload->desc()));
            workload->run();
            if (!workload->verify())


I was wondering whether it is C++ style that callback function should start with on, it looks more like Java style. If the callbacks all use on as a prefix, then I think that including verify and run also need to be prefixed with on?

IMO, a function that does something is better to be named as an action.
I expected the function "failed" to be a function that returns a status that the worker is failed or not until I look at the implementation.
And I expected the function "result" to be a function that returns a string or something that represents a "result". But actually, it dumps the result inside the function.

dbms/src/Storages/Page/tests/stress/PSWorkload.h

JaySon-Huang · 2021-09-13T04:30:52Z

Actually, the following 4 points are checked by the clang-tidy:

Check the explicit in class construction
Check const in function which in class
Do not use (int) to forced transfer type
Case-check-in name
- Camel-Case for class, struct, function, type aliasing name
- Snake-Case for member variables, local variables
- Do not add prefix/suffix for member variables (for example, prefix "m_" or suffix "_")

We introduce clang-tidy into CI by #2834, but now it is temporarily disabled since there are too many warnings

Check the semicolon after the class member functions

Seems that clang-tidy can not check it either 😂

jiaqizho · 2021-09-13T04:46:46Z

Actually, the following 4 points are checked by the clang-tidy:

Check the explicit in class construction

Check const in function which in class

Do not use (int) to forced transfer type

Case-check-in name

Camel-Case for class, struct, function, type aliasing name

Snake-Case for member variables, local variables

Do not add prefix/suffix for member variables (for example, prefix "m_" or suffix "_")

We introduce clang-tidy into CI by #2834, but now it is temporarily disabled since there are too many warnings

Check the semicolon after the class member functions
Seems that clang-tidy can not check it either 😂

great to know that 😂.

Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>

jiaqizho · 2021-09-14T03:02:32Z

LGTM

Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>

ti-chi-bot · 2021-09-14T07:44:49Z

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

Details

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.

Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>

JaySon-Huang · 2021-09-14T08:01:01Z

/merge

ti-chi-bot · 2021-09-14T08:01:01Z

@JaySon-Huang: 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.

Details

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 · 2021-09-14T08:01:06Z

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

Details

Commit hash: dc3c71b

JaySon-Huang added 6 commits September 9, 2021 15:29

Fix clang-tidy lints && some style

6d85de3

Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>

Add some comment

b514fc4

Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>

Adjust some interface for Workload

ce54a7b

Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>

Out of scope format fix

aabd2be

Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>

Replace fmt::print with Logger

372fef7

Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>

Fix use after free

955c198

Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>

JaySon-Huang requested a review from jiaqizho September 9, 2021 09:41

ti-chi-bot added 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 Sep 9, 2021

Merge branch 'master' into refine_page_stress

35406ad

JaySon-Huang commented Sep 9, 2021

View reviewed changes

Fix some docs

32e116d

Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>

JaySon-Huang mentioned this pull request Sep 9, 2021

fix stress-page-storage use after free problem #3061

Closed

JaySon-Huang added the type/testing Issue or PR for testing label Sep 9, 2021

jiaqizho reviewed Sep 10, 2021

View reviewed changes

Define interrupt by signal as failure

f5cd1b5

Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>

jiaqizho mentioned this pull request Sep 14, 2021

Add more workload into page storage stress test #3057

Merged

7 tasks

JaySon-Huang self-assigned this Sep 14, 2021

JaySon-Huang added 4 commits September 14, 2021 14:12

Add back some access specifiers

93b73d8

Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>

Cleanup

5969cba

Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>

Merge remote-tracking branch 'upstream/master' into refine_page_stress

bd5e360

Fix all lints

05d8938

Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>

jiaqizho approved these changes Sep 14, 2021

View reviewed changes

Format

dc3c71b

Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>

lidezhu approved these changes Sep 14, 2021

View reviewed changes

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

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

jiaqizho approved these changes Sep 14, 2021

View reviewed changes

ti-chi-bot merged commit 9741f06 into pingcap:master Sep 14, 2021

JaySon-Huang deleted the refine_page_stress branch September 14, 2021 09:36

Conversation

JaySon-Huang commented Sep 9, 2021

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

Uh oh!

ti-chi-bot commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JaySon-Huang Sep 9, 2021

Choose a reason for hiding this comment

Uh oh!

jiaqizho left a comment

Choose a reason for hiding this comment

Uh oh!

jiaqizho Sep 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiaqizho Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

JaySon-Huang Sep 13, 2021

Choose a reason for hiding this comment

Uh oh!

jiaqizho Sep 14, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JaySon-Huang commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jiaqizho commented Sep 13, 2021

Uh oh!

jiaqizho commented Sep 14, 2021

Uh oh!

ti-chi-bot commented Sep 14, 2021

Uh oh!

JaySon-Huang commented Sep 14, 2021

Uh oh!

ti-chi-bot commented Sep 14, 2021

Uh oh!

ti-chi-bot commented Sep 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ti-chi-bot commented Sep 9, 2021 •

edited

Loading

jiaqizho Sep 10, 2021 •

edited

Loading

JaySon-Huang commented Sep 13, 2021 •

edited

Loading