Skip to content

PageStorage: implement BlobStore IO#3576

Merged
ti-chi-bot merged 59 commits intopingcap:masterfrom
jiaqizho:add-blob-store
Dec 16, 2021
Merged

PageStorage: implement BlobStore IO#3576
ti-chi-bot merged 59 commits intopingcap:masterfrom
jiaqizho:add-blob-store

Conversation

@jiaqizho
Copy link
Contributor

@jiaqizho jiaqizho commented Dec 1, 2021 •
edited
Loading

What problem does this PR solve?

Issue Number: related to #3594

Problem Summary:

What is changed and how it works?

need to do in next PR:

  • GC
  • multi-path support

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

@jiaqizho jiaqizho requested a review from JaySon-Huang December 1, 2021 08:08
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Dec 1, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • JaySon-Huang

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.

@ti-chi-bot 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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 1, 2021
@jiaqizho
Copy link
Contributor Author

jiaqizho commented Dec 1, 2021

@JaySon-Huang PTAL
This is a semi-finished PR. It is only used to discuss whether the interface and structure are reasonable.

Please don't make any suggestions on the details.

JaySon-Huang
JaySon-Huang reviewed Dec 1, 2021
View reviewed changes
JaySon-Huang
JaySon-Huang reviewed Dec 3, 2021
View reviewed changes
flowbehappy
flowbehappy reviewed Dec 6, 2021
View reviewed changes
JaySon-Huang
JaySon-Huang reviewed Dec 14, 2021
View reviewed changes
@jiaqizho
Copy link
Contributor Author

jiaqizho commented Dec 15, 2021

/run-all-tests

JaySon-Huang
JaySon-Huang reviewed Dec 15, 2021
View reviewed changes
JaySon-Huang
JaySon-Huang reviewed Dec 15, 2021
View reviewed changes
@sre-bot
Copy link
Collaborator

sre-bot commented Dec 15, 2021

Coverage detail: https://ci-internal.pingcap.net/job/tics_ghpr_unit_test/376/cobertura/
(Coverage detail url is limited office network access)

lines: 42.2% (46938 out of 111258)
branches: 5.8% (70228 out of 1203620)

JaySon-Huang
JaySon-Huang reviewed Dec 16, 2021
View reviewed changes
dbms/src/Storages/Page/V3/BlobStore.cpp
{
if (!smap->markFree(offset, buf_size))
{
smap->logStats();
Copy link
Contributor

@JaySon-Huang JaySon-Huang Dec 16, 2021

Choose a reason for hiding this comment

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

ditto

JaySon-Huang
JaySon-Huang reviewed Dec 16, 2021
View reviewed changes
dbms/src/Storages/Page/V3/BlobStore.cpp
// Can't insert into this spacemap
if (offset == INVALID_BLOBFILE_OFFSET)
{
stat->smap->logStats();
Copy link
Contributor

@JaySon-Huang JaySon-Huang Dec 16, 2021

Choose a reason for hiding this comment

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

Exceptions thrown will be logged to another line, if TiFlash is busy and log many other messages, it would be hard for you to locate problems. And you just use LOG_DEBUG in logStats, which would be ignored when users use a higher log level.

JaySon-Huang
JaySon-Huang reviewed Dec 16, 2021
View reviewed changes
JaySon-Huang
JaySon-Huang approved these changes Dec 16, 2021
View reviewed changes
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 with small comment

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 16, 2021
jiaqizho and others added 2 commits December 16, 2021 17:32
@jiaqizho @JaySon-Huang
Update dbms/src/Storages/Page/PageUtil.h
d904ae5 
Co-authored-by: JaySon <jayson.hjs@gmail.com>
@jiaqizho @JaySon-Huang
Update dbms/src/Storages/Page/PageUtil.h
a43704e 
Co-authored-by: JaySon <jayson.hjs@gmail.com>
@jiaqizho
Copy link
Contributor Author

jiaqizho commented Dec 16, 2021

/merge

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Dec 16, 2021

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

ti-chi-bot commented Dec 16, 2021

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

DetailsCommit hash: a43704e

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

sre-bot commented Dec 16, 2021

Coverage detail: https://ci-internal.pingcap.net/job/tics_ghpr_unit_test/409/cobertura/
(Coverage detail url is limited office network access)

lines: 42.6% (47522 out of 111638)
branches: 6.0% (73018 out of 1207985)

@JaySon-Huang
Copy link
Contributor

JaySon-Huang commented Dec 16, 2021

/run-integration-test

@sre-bot
Copy link
Collaborator

sre-bot commented Dec 16, 2021

Coverage detail: https://ci-internal.pingcap.net/job/tics_ghpr_unit_test/416/cobertura/
(Coverage detail url is limited office network access)

lines: 42.6% (47522 out of 111653)
branches: 6.0% (73012 out of 1208103)

@ti-chi-bot ti-chi-bot merged commit 7b61a52 into pingcap:master Dec 16, 2021
@JaySon-Huang JaySon-Huang deleted the add-blob-store branch December 17, 2021 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@flowbehappy flowbehappy flowbehappy left review comments

@JaySon-Huang JaySon-Huang JaySon-Huang approved these changes

Assignees

No one assigned

Labels

release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jiaqizho @ti-chi-bot @sre-bot @JaySon-Huang @flowbehappy