Refine the GC interface by `PageEntriesEdit` #3879

JaySon-Huang · 2022-01-17T09:17:59Z

What problem does this PR solve?

Issue Number: a part of #3594; a part of WALStore implementation

Problem Summary:

Define WALStore interface
Use PageEntriesEdit to unify the data exchange structure between BlobStore and PageDirectory

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

ti-chi-bot · 2022-01-17T09:18:00Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

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.

JaySon-Huang · 2022-01-17T09:18:19Z

/run-all-tests

dbms/src/Storages/Page/V3/BlobStore.cpp

dbms/src/Storages/Page/V3/PageStorageImpl.cpp

jiaqizho · 2022-01-18T06:34:11Z

dbms/src/Storages/Page/V3/WALStore.cpp

+    throw Exception("Not implemented", ErrorCodes::NOT_IMPLEMENTED);
+}
+
+void WALStore::gc()


not sure WALStore need gc or we can make it more simple?

There are two approaches:

Stopping the foreground write and dump the MVCCDirectory into a new LogFile to replace all files before it

Select some LogFiles and compact them in a background thread

I prefer 2 cause it doesn't stall the foreground writes

While 1 will stop the foreground writes, and the stop duration is proportional to the size of MVCC Directory.

jiaqizho

It's just a suggestion, can we finish the WalStore in this pr, in this case, the changes to the interface can be done together?

jiaqizho · 2022-01-19T02:41:27Z

dbms/src/Storages/Page/V3/BlobStore.cpp

@@ -90,7 +91,6 @@ PageEntriesEdit BlobStore::write(DB::WriteBatch & wb, const WriteLimiterPtr & wr
        switch (write.type)
        {
        case WriteBatch::WriteType::PUT:
-        case WriteBatch::WriteType::UPSERT:


why removed UPSERT?

After digging into current gc routine implementation, we only use PageEntriesEdit:: upsertPage in BlobStore::gc (with version set in the same time). WriteBatch::upsert is not a valid input.
https://github.com/pingcap/tics/blob/ded8a062b9f777ccfd70087f6341feac725bcb08/dbms/src/Storages/Page/V3/BlobStore.cpp#L406

jiaqizho · 2022-01-19T02:43:20Z

dbms/src/Storages/Page/V3/PageDirectory.cpp

-    // stage 1, persisted the changes to WAL
-    // wal.apply(edit);
+    // stage 1, persisted the changes to WAL with version [seq=last_seq + 1, epoch=0]
+    // wal->apply(edit, PageVersionType(last_sequence + 1, 0));


so we do have wal->apply already , can we just use it? or is there remain some work for it?

We need to set a proper version for edits written from PageDirectory::apply...
https://github.com/pingcap/tics/blob/ded8a062b9f777ccfd70087f6341feac725bcb08/dbms/src/Storages/Page/V3/WALStore.cpp#L136-L142

... While in PageDirectory::gcApply, we use the existing version
https://github.com/pingcap/tics/blob/ded8a062b9f777ccfd70087f6341feac725bcb08/dbms/src/Storages/Page/V3/PageDirectory.cpp#L279-L300

jiaqizho · 2022-01-19T02:47:21Z

dbms/src/Storages/Page/V3/PageDirectory.h

@@ -159,7 +159,7 @@ class PageDirectory

    mutable std::list<std::weak_ptr<PageDirectorySnapshot>> snapshots;

-    WALStore wal;
+    WALStorePtr wal;


I am not sure we need ptr to refine WALStore.

And I don't see we init it in the constructor?

I use WALStore::create to create it in restore instead of during the constructor of PageDirectory::PageDirectory now, so it need to be a ptr.
I may refactor PageDirectory to make it construct by a static method create, so that we do not get a PageDirectory instance that is created but without restoring existing data from disks.

JaySon-Huang · 2022-01-19T05:57:18Z

It's just a suggestion, can we finish the WalStore in this pr, in this case, the changes to the interface can be done together?

I've created #3891 for the full implementation of WALStore. That PR contains lots of changes files (1300 lines over 23 files).
The changes in this PR are mainly changing BlobStore, PageDirectory. Making it a standalone PR could make #3891 more clean.

Signed-off-by: JaySon-Huang <tshent@qq.com>

JaySon-Huang · 2022-01-19T12:12:09Z

Should get #3897 merged first.

Signed-off-by: JaySon-Huang <tshent@qq.com>

JaySon-Huang · 2022-01-20T04:14:45Z

/merge

ti-chi-bot · 2022-01-20T04:14:46Z

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

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-01-20T04:14:52Z

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

Commit hash: 4057c79

sre-bot · 2022-01-20T04:43:29Z

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

lines: 44.7% (51509 out of 115198)
branches: 7.0% (89829 out of 1275287)

ti-chi-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Jan 17, 2022

JaySon-Huang requested a review from jiaqizho January 17, 2022 09:18

ti-chi-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 17, 2022

JaySon-Huang commented Jan 17, 2022

View reviewed changes

dbms/src/Storages/Page/V3/BlobStore.cpp Show resolved Hide resolved

dbms/src/Storages/Page/V3/PageStorageImpl.cpp Outdated Show resolved Hide resolved

jiaqizho reviewed Jan 18, 2022

View reviewed changes

jiaqizho reviewed Jan 19, 2022

View reviewed changes

JaySon-Huang force-pushed the refine_gc_interface branch from 7487e4f to 40d6c35 Compare January 19, 2022 11:50

JaySon-Huang added 5 commits January 19, 2022 19:56

Fix lock on blob store

eae7b90

Signed-off-by: JaySon-Huang <tshent@qq.com>

Fix lock on PageDirectory

ca65a7b

Signed-off-by: JaySon-Huang <tshent@qq.com>

Refine some interface

7382701

Signed-off-by: JaySon-Huang <tshent@qq.com>

Add interface of WALStore

679a0ed

Signed-off-by: JaySon-Huang <tshent@qq.com>

Add assertion test

fd4bbbb

Signed-off-by: JaySon-Huang <tshent@qq.com>

JaySon-Huang force-pushed the refine_gc_interface branch from 40d6c35 to fd4bbbb Compare January 19, 2022 12:09

jiaqizho approved these changes Jan 20, 2022

View reviewed changes

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

Cleanup walstore.cpp

4057c79

Signed-off-by: JaySon-Huang <tshent@qq.com>

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

ti-chi-bot merged commit a497442 into pingcap:master Jan 20, 2022

JaySon-Huang deleted the refine_gc_interface branch January 20, 2022 05:12

JaySon-Huang mentioned this pull request Jan 20, 2022

PageStorage: Fix locks on BlobStore, PageDirectory #3897

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine the GC interface by `PageEntriesEdit` #3879

Refine the GC interface by `PageEntriesEdit` #3879

JaySon-Huang commented Jan 17, 2022

ti-chi-bot commented Jan 17, 2022 •

edited

Loading

JaySon-Huang commented Jan 17, 2022

jiaqizho Jan 18, 2022

JaySon-Huang Jan 18, 2022

JaySon-Huang Jan 18, 2022

jiaqizho left a comment

jiaqizho Jan 19, 2022

JaySon-Huang Jan 19, 2022

jiaqizho Jan 19, 2022

JaySon-Huang Jan 19, 2022 •

edited

Loading

jiaqizho Jan 19, 2022

JaySon-Huang Jan 19, 2022 •

edited

Loading

JaySon-Huang commented Jan 19, 2022

JaySon-Huang commented Jan 19, 2022

JaySon-Huang commented Jan 20, 2022

ti-chi-bot commented Jan 20, 2022

ti-chi-bot commented Jan 20, 2022

sre-bot commented Jan 20, 2022

Refine the GC interface by PageEntriesEdit #3879

Refine the GC interface by PageEntriesEdit #3879

Conversation

JaySon-Huang commented Jan 17, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Jan 17, 2022 • edited Loading

JaySon-Huang commented Jan 17, 2022

jiaqizho Jan 18, 2022

Choose a reason for hiding this comment

JaySon-Huang Jan 18, 2022

Choose a reason for hiding this comment

JaySon-Huang Jan 18, 2022

Choose a reason for hiding this comment

jiaqizho left a comment

Choose a reason for hiding this comment

jiaqizho Jan 19, 2022

Choose a reason for hiding this comment

JaySon-Huang Jan 19, 2022

Choose a reason for hiding this comment

jiaqizho Jan 19, 2022

Choose a reason for hiding this comment

JaySon-Huang Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

jiaqizho Jan 19, 2022

Choose a reason for hiding this comment

JaySon-Huang Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

JaySon-Huang commented Jan 19, 2022

JaySon-Huang commented Jan 19, 2022

JaySon-Huang commented Jan 20, 2022

ti-chi-bot commented Jan 20, 2022

ti-chi-bot commented Jan 20, 2022

sre-bot commented Jan 20, 2022

Refine the GC interface by `PageEntriesEdit` #3879

Refine the GC interface by `PageEntriesEdit` #3879

ti-chi-bot commented Jan 17, 2022 •

edited

Loading

JaySon-Huang Jan 19, 2022 •

edited

Loading

JaySon-Huang Jan 19, 2022 •

edited

Loading