feat: add page_stress_testing as a subcommand of tiflash #5038

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 <[email protected]>

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.

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 <[email protected]>

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

/build

Signed-off-by: Lloyd-Pottiger <[email protected]>

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

/build

Signed-off-by: Lloyd-Pottiger <[email protected]>

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 <[email protected]>

dbms/src/Server/CMakeLists.txt

Signed-off-by: Lloyd-Pottiger <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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.

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.

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.

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 <[email protected]>

fix clang-tidy error

e7cf388

Signed-off-by: Lloyd-Pottiger <[email protected]>

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 <[email protected]>

add mainEntry.cpp

d3be4aa

Signed-off-by: Lloyd-Pottiger <[email protected]>

change directory layout and add CMakeLists.txt

5a36b3f

Signed-off-by: Lloyd-Pottiger <[email protected]>

add it to main.cpp

8ba638c

Signed-off-by: Lloyd-Pottiger <[email protected]>

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 <[email protected]>

remove page_storage_v3 in workload CMakeLists

79e1dee

Signed-off-by: Lloyd-Pottiger <[email protected]>

explicit trigger REGISTER_WORKLOAD

e80288d

Signed-off-by: Lloyd-Pottiger <[email protected]>

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

fix tidy error

3a3f46a

Signed-off-by: Lloyd-Pottiger <[email protected]>

fix tidy error

28cc416

Signed-off-by: Lloyd-Pottiger <[email protected]>

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 jiaqizho, JaySon-Huang, JinheLin and lilinghai June 1, 2022 10:16

change CMakeLists

9124d5d

Signed-off-by: Lloyd-Pottiger <[email protected]>

JinheLin reviewed Jun 2, 2022

View reviewed changes

dbms/src/Storages/Page/stress/workload/MainEntry.cpp Outdated 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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

rollback irrelevant changes

09c139c

Signed-off-by: Lloyd-Pottiger <[email protected]>

jiaqizho reviewed Jun 9, 2022

View reviewed changes

remove ENABLE_V3_PAGESTORAGE

201ddbf

Signed-off-by: Lloyd-Pottiger <[email protected]>

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 <[email protected]>

remove useless code

fdee8de

Signed-off-by: Lloyd-Pottiger <[email protected]>

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 <[email protected]>

clear clang-tidy error

247ae77

Signed-off-by: Lloyd-Pottiger <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add page_stress_testing as a subcommand of tiflash #5038

feat: add page_stress_testing as a subcommand of tiflash #5038

Lloyd-Pottiger commented May 31, 2022 •

edited

Loading

ti-chi-bot commented May 31, 2022 •

edited

Loading

Lloyd-Pottiger commented May 31, 2022

Lloyd-Pottiger commented Jun 1, 2022

Lloyd-Pottiger commented Jun 1, 2022 •

edited

Loading

jiaqizho Jun 9, 2022

Lloyd-Pottiger Jun 9, 2022

JaySon-Huang Jun 9, 2022

JaySon-Huang Jun 9, 2022 •

edited

Loading

Lloyd-Pottiger Jun 9, 2022

JaySon-Huang Jun 9, 2022

Lloyd-Pottiger Jun 9, 2022

JinheLin Jun 9, 2022

Lloyd-Pottiger Jun 9, 2022

jiaqizho Jun 9, 2022

Lloyd-Pottiger Jun 9, 2022

JinheLin Jun 9, 2022

Lloyd-Pottiger Jun 9, 2022

JaySon-Huang Jun 9, 2022

Lloyd-Pottiger Jun 9, 2022

JaySon-Huang left a comment

jiaqizho left a comment

Lloyd-Pottiger commented Jun 10, 2022

ti-chi-bot commented Jun 10, 2022

ti-chi-bot commented Jun 10, 2022

ti-chi-bot commented Jun 10, 2022

sre-bot commented Jun 10, 2022

	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)

feat: add page_stress_testing as a subcommand of tiflash #5038

feat: add page_stress_testing as a subcommand of tiflash #5038

Conversation

Lloyd-Pottiger commented May 31, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented May 31, 2022 • edited Loading

Lloyd-Pottiger commented May 31, 2022

Lloyd-Pottiger commented Jun 1, 2022

Lloyd-Pottiger commented Jun 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaySon-Huang Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaySon-Huang left a comment

Choose a reason for hiding this comment

jiaqizho left a comment

Choose a reason for hiding this comment

Lloyd-Pottiger commented Jun 10, 2022

ti-chi-bot commented Jun 10, 2022

ti-chi-bot commented Jun 10, 2022

ti-chi-bot commented Jun 10, 2022

sre-bot commented Jun 10, 2022

Coverage for changed files

Coverage summary

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