Skip to content

Fix broken page storage stress testing#2297

Merged
ti-srebot merged 4 commits intopingcap:masterfrom
JaySon-Huang:fix_page_storage_compile
Jul 23, 2021
Merged

Fix broken page storage stress testing#2297
ti-srebot merged 4 commits intopingcap:masterfrom
JaySon-Huang:fix_page_storage_compile

Conversation

@JaySon-Huang
Copy link
Contributor

@JaySon-Huang JaySon-Huang commented Jul 1, 2021

What problem does this PR solve?

Issue Number: related to #2296

This stress testing helped us to locate the error of #2249 and #2461

What is changed and how it works?

Depending on #2296.

  • Fix the broken test and add it to CI build
    • may add it as a CI test later.
  • Use PageStorage::gc(bool not_skip = false, const RateLimiterPtr & rate_limiter); instead of PageStorage::gc(const Context& global_context, bool not_skip = false); so that @JinheLin
    • PageStorage does not rely on DB::Context we can speed up some building
    • It is reasonable that GC will only be called in one thread and use the same RateLimiter

But still page_storage rely on some libraries that are useless and slow down the compiling (kv_client tipb for example). I will file another PR to make it more independent. (Mainly, we need to get some global object out of DB::Context)

Related changes

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

Check List

Tests

  • Integration test
    • integrate with CI building

Side effects

Release note

  • No release note

@JaySon-Huang JaySon-Huang mentioned this pull request Jul 1, 2021
3 tasks
@JaySon-Huang JaySon-Huang force-pushed the fix_page_storage_compile branch from 7d1162c to d5b6ed0 Compare July 1, 2021 10:19
@jiangyuzhao jiangyuzhao force-pushed the fix_page_storage_compile branch from 298be1c to 44ee868 Compare July 5, 2021 19:09
@JaySon-Huang JaySon-Huang force-pushed the fix_page_storage_compile branch 2 times, most recently from 756bc35 to f680b0b Compare July 6, 2021 08:56
@JaySon-Huang JaySon-Huang self-assigned this Jul 15, 2021
@JaySon-Huang JaySon-Huang changed the title Fix broken page storage stress testing [DNM] Fix broken page storage stress testing Jul 15, 2021
@JaySon-Huang JaySon-Huang force-pushed the fix_page_storage_compile branch 3 times, most recently from fa134bf to 77fa7f0 Compare July 21, 2021 13:43
@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

@JaySon-Huang JaySon-Huang changed the title [DNM] Fix broken page storage stress testing Fix broken page storage stress testing Jul 21, 2021
Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>
Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>
@JaySon-Huang JaySon-Huang force-pushed the fix_page_storage_compile branch from fbfc1cf to 9161bdb Compare July 23, 2021 03:51
@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

@JaySon-Huang JaySon-Huang added the type/testing Issue or PR for testing label Jul 23, 2021
@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

@jiaqizho
Copy link
Contributor

LGTM

@ti-srebot
Copy link
Collaborator

@jiaqizho, Thanks for your review. The bot only counts LGTMs from Reviewers and higher roles, but you're still welcome to leave your comments. See the corresponding SIG page for more information. Related SIG: tiflash(slack).

opt.clean_before_run = options["clean_before_run"].as<bool>();
opt.timeout_s = options["timeout"].as<UInt32>();
opt.read_delay_ms = options["read_delay_ms"].as<UInt32>();
opt.num_writer_slots = options["writer_slots"].as<UInt32>();
Copy link
Contributor

Choose a reason for hiding this comment

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

should check num_writer_slots same as num_writers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, they can be different.
The num_writer_slots is the pending number of PageFiles that PageStorage can write in parallel. The num_writers is the number of write threads that call PageStorage::write.
num_writers can be greater that num_writer_slots, but finally the throughput is limited by num_writer_slots

Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>
@ti-srebot
Copy link
Collaborator

@JinheLin, Thanks for your review. The bot only counts LGTMs from Reviewers and higher roles, but you're still welcome to leave your comments. See the corresponding SIG page for more information. Related SIG: tiflash(slack).

@JaySon-Huang
Copy link
Contributor Author

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 23, 2021
@ti-srebot
Copy link
Collaborator

/run-all-tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status/can-merge Indicates a PR has been approved by a committer. type/testing Issue or PR for testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants