feat: add page_stress_testing as a subcommand of tiflash by Lloyd-Pottiger · Pull Request #5038 · pingcap/tiflash

Lloyd-Pottiger · 2022-05-31T08:42:23Z

What problem does this PR solve?

Issue Number: close #5037

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

Release note

None

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

ti-chi-bot · 2022-05-31T08:42:25Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JaySon-Huang
jiaqizho

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.

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

Lloyd-Pottiger · 2022-05-31T13:58:58Z

/build

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

Lloyd-Pottiger · 2022-06-01T08:27:43Z

/build

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

Lloyd-Pottiger · 2022-06-01T10:32:25Z

run like:

./dbms/src/Server/tiflash stressworkload -M 0 -I true -V 3 -P stress -T 120
# or
ninja stress-workload
./dbms/src/Storages/Page/stress/workload/stress-workload -M 0 -I true -V 3 -P stress -T 120

dbms/src/Storages/Page/stress/workload/MainEntry.cpp

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

dbms/src/Server/CMakeLists.txt

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

dbms/src/Storages/CMakeLists.txt

jiaqizho · 2022-06-09T02:59:04Z

dbms/src/Storages/Page/stress/workload/MainEntry.cpp

+int StressWorkload::mainEntry(int argc, char ** argv)
+{
+    {
+        // maybe due to sequence of linking, REGISTER_WORKLOAD is not visible to main function in dbms/src/Server/main.cpp


why __attribute__((constructor)) won't triggered before mainEntry?

just as the comment says, maybe due to sequence of linking, REGISTER_WORKLOAD is not visible to main function in dbms/src/Server/main.cpp.
when we run in this way:

./dbms/src/Server/tiflash pageworkload -M 0 -I true -V 3 -P stress -T 120

__attribute__((constructor)) won't be triggered.
we run in this way:

ninja page-workload ./dbms/src/Storages/Page/stress/workload/page-workload -M 0 -I true -V 3 -P stress -T 120

it will be triggered.
I still could not find the detail reason. 😢

Maybe change the registration to be explicit like the function registerStorages? __attribute__((constructor)) is not a common way and depends on specific compiler behavior.

tiflash/dbms/src/Storages/registerStorages.cpp

Lines 38 to 56 in 5b61ae7

void registerStorages()

{

auto & factory = StorageFactory::instance();

registerStorageLog(factory);

registerStorageTinyLog(factory);

registerStorageDeltaMerge(factory);

registerStorageStripeLog(factory);

registerStorageNull(factory);

registerStorageMerge(factory);

registerStorageBuffer(factory);

registerStorageMemory(factory);

registerStorageFile(factory);

registerStorageDictionary(factory);

registerStorageSet(factory);

registerStorageJoin(factory);

registerStorageView(factory);

registerStorageMaterializedView(factory);

}

I still could not find the detail reason. 😢

__attribute__((constructor)) doesn't work when we compile those files into a static lib https://stackoverflow.com/questions/1202494/why-doesnt-attribute-constructor-work-in-a-static-library

Maybe change the registration to be explicit like the function registerStorages? __attribute__((constructor)) is not a common way and depends on specific compiler behavior.

That's how we've settled it. explicitly trigger them.

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

JaySon-Huang · 2022-06-09T03:39:22Z

dbms/src/Storages/Page/stress/workload/CMakeLists.txt

+add_executable (page-workload Main.cpp ${page-workload-src})
+target_link_libraries (page-workload dbms clickhouse_functions clickhouse-server-lib)
+target_compile_options (page-workload PRIVATE -Wno-format -lc++)


Seems the binary page_workload is useless. Do you think we still need this standalone binary? @jiaqizho

It would increase the compile time of tiflash but can decrease the compile when we just want to run page-workload only. So maybe we can keep it just like dt-workload?

In fact, I want to remove the binary of dt-workload...

ok, I will remove both of them.

ok for remove it.

Please delete useless code files as well： https://github.com/pingcap/tiflash/blob/master/dbms/src/Storages/DeltaMerge/tools/workload/Main.cpp

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

JaySon-Huang · 2022-06-09T07:41:16Z

dbms/src/Storages/Page/stress/CMakeLists.txt

+include_directories (${CMAKE_CURRENT_BINARY_DIR})
+
+add_subdirectory (workload EXCLUDE_FROM_ALL)


Do we still need to keep this "stress" directory? Maybe move "dbms/src/Storages/Page/stress/workload" -> "dbms/src/Storages/Page/workload"?

removed, the same as dbms/src/Storages/DeltaMerge/workload

JaySon-Huang

LGTM with minor comment

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

jiaqizho

LGTM

Lloyd-Pottiger · 2022-06-10T02:29:10Z

/merge

ti-chi-bot · 2022-06-10T02:29:10Z

@Lloyd-Pottiger: 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 · 2022-06-10T02:29:12Z

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

Details

Commit hash: 247ae77

ti-chi-bot · 2022-06-10T02:29:25Z

@Lloyd-Pottiger: 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.

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.

sre-bot · 2022-06-10T02:45:25Z

Coverage for changed files

too many lines from llvm-cov, please refer to full report instead

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18328      9657             47.31%    205678  96722        52.97%

full coverage report (for internal network access only)

Lloyd-Pottiger added 2 commits May 31, 2022 14:21

move stree workload under namespace DB::PS::tests

5f8096d

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

fix clang-tidy error

e7cf388

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 31, 2022

Lloyd-Pottiger added 4 commits May 31, 2022 16:45

fix clang-tidy error

75c0c86

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

add mainEntry.cpp

d3be4aa

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

change directory layout and add CMakeLists.txt

5a36b3f

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

add it to main.cpp

8ba638c

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

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

Lloyd-Pottiger added 3 commits June 1, 2022 14:04

fix some known errors

f266aaa

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

remove page_storage_v3 in workload CMakeLists

79e1dee

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

explicit trigger REGISTER_WORKLOAD

e80288d

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

Lloyd-Pottiger added 2 commits June 1, 2022 17:49

fix tidy error

3a3f46a

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

fix tidy error

28cc416

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

Lloyd-Pottiger changed the title ~~[WIP]feat: add page_stress_testing as a subcommand of tiflash~~ feat: add page_stress_testing as a subcommand of tiflash Jun 1, 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 Jun 1, 2022

Lloyd-Pottiger requested review from JaySon-Huang, JinheLin, jiaqizho and lilinghai June 1, 2022 10:16

change CMakeLists

9124d5d

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

JinheLin reviewed Jun 2, 2022

View reviewed changes

dbms/src/Storages/Page/stress/workload/MainEntry.cpp Show resolved Hide resolved

dbms/src/Storages/Page/stress/workload/MainEntry.cpp Outdated Show resolved Hide resolved

add some comments about trigger register

cb478ef

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 2, 2022

Merge branch 'master' into add-page-stress-as-a-command

9473123

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 2, 2022

reslove conflicts

5e5e06c

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

JaySon-Huang reviewed Jun 6, 2022

View reviewed changes

dbms/src/Server/CMakeLists.txt Outdated Show resolved Hide resolved

Lloyd-Pottiger added 2 commits June 7, 2022 10:21

rename stressworkload to pageworkload

c406056

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

rollback irrelevant changes

09c139c

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

jiaqizho reviewed Jun 9, 2022

View reviewed changes

remove ENABLE_V3_PAGESTORAGE

201ddbf

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

JaySon-Huang reviewed Jun 9, 2022

View reviewed changes

Lloyd-Pottiger added 2 commits June 9, 2022 15:00

remove dt-workload & page-workload executable target

5f8afb2

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

remove useless code

fdee8de

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

JaySon-Huang reviewed Jun 9, 2022

View reviewed changes

JaySon-Huang approved these changes Jun 9, 2022

View reviewed changes

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

Lloyd-Pottiger added 2 commits June 9, 2022 16:13

remove stress & tools dir

5394c4b

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

clear clang-tidy error

247ae77

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

jiaqizho approved these changes Jun 10, 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 Jun 10, 2022

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

Merge branch 'master' into add-page-stress-as-a-command

88fb2d6

ti-chi-bot merged commit a9b322a into pingcap:master Jun 10, 2022

Lloyd-Pottiger deleted the add-page-stress-as-a-command branch June 10, 2022 02:57

	void registerStorages()
	{
	auto & factory = StorageFactory::instance();

	registerStorageLog(factory);
	registerStorageTinyLog(factory);
	registerStorageDeltaMerge(factory);
	registerStorageStripeLog(factory);
	registerStorageNull(factory);
	registerStorageMerge(factory);
	registerStorageBuffer(factory);
	registerStorageMemory(factory);
	registerStorageFile(factory);
	registerStorageDictionary(factory);
	registerStorageSet(factory);
	registerStorageJoin(factory);
	registerStorageView(factory);
	registerStorageMaterializedView(factory);
	}

		include_directories (${CMAKE_CURRENT_BINARY_DIR})

		add_subdirectory (workload EXCLUDE_FROM_ALL)

Conversation

Lloyd-Pottiger commented May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot commented May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lloyd-Pottiger commented May 31, 2022

Uh oh!

Lloyd-Pottiger commented Jun 1, 2022

Uh oh!

Lloyd-Pottiger commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JaySon-Huang Jun 9, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JaySon-Huang left a comment

Choose a reason for hiding this comment

Uh oh!

jiaqizho left a comment

Choose a reason for hiding this comment

Uh oh!

Lloyd-Pottiger commented Jun 10, 2022

Uh oh!

ti-chi-bot commented Jun 10, 2022

Uh oh!

ti-chi-bot commented Jun 10, 2022

Uh oh!

ti-chi-bot commented Jun 10, 2022

Uh oh!

sre-bot commented Jun 10, 2022

Coverage for changed files

Coverage summary

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Lloyd-Pottiger commented May 31, 2022 •

edited

Loading

ti-chi-bot commented May 31, 2022 •

edited

Loading

Lloyd-Pottiger commented Jun 1, 2022 •

edited

Loading

JaySon-Huang Jun 9, 2022 •

edited

Loading