support keyspace feature #6816

iosmanthus · 2023-02-14T03:49:25Z

Signed-off-by: iosmanthus myosmanthustree@gmail.com

What problem does this PR solve?

Issue Number: close #6815

TiKV API v2: https://tikv.org/docs/dev/concepts/explore-tikv-features/api-v2/

Problem Summary:
Deps:

S3 remote read: support storing all data in UniversalPageStorage #6717
client-c: Support keyspace requests tikv/client-c#123
kvproto:

What is changed and how it works?

This pull request separates each keyspace's db/tables by rewriting their name with a keyspace prefix.
For example, table 1 in keyspace 2 is stored in a directory named: ks_2_t_1, and the same as the schema SQL file.

The keyspace's information is decoded from the region range in the writing process, and from gRPC requests context in the reading process. Every time TiFlash syncSchema, it needs a keyspace id parameter. The persistent schema will also contain the keyspace id in case of restarting the server.

This pull request also introduces a new config named api-version, whose default value is 1 meaning the node is working in the legacy mode. In this mode, schema info will be fetched in the bootstrap stage. If the api-version is 2, then the node is working in the keyspace mode, which will fetch schema lazily.

The pull request does not support the PageStoreV3 format and needs to be tested in V2 for now. In the future, the pull request should be compatible with the UniversalPageStore feature.

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

support keyspace feature

ti-chi-bot · 2023-02-14T03:49:26Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JaySon-Huang
flowbehappy

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.

sre-bot · 2023-02-14T03:49:31Z

All committers have signed the CLA.

dbms/src/Interpreters/IDAsPathUpgrader.cpp

yongman · 2023-02-15T03:24:30Z

dbms/src/Storages/Transaction/TMTStorages.cpp

-    storages.emplace(table_id, storage);
+    storages.emplace(keyspace_table_id, storage);
+    auto [it, _] = keyspaces.try_emplace(keyspace_id, 0);
+    it->second++;


What is this counter used for？

Oh, I get it.

dbms/src/Flash/Management/ManualCompact.cpp

dbms/src/Storages/GCManager.cpp

JaySon-Huang · 2023-02-16T03:02:09Z

dbms/src/Storages/Transaction/TiDB.cpp

+    // If the table_info_json has no keyspace id, we use the keyspace_id_ as the default value.
+    if (keyspace_id == NullspaceID)
+    {
+        keyspace_id = keyspace_id_;
+    }


Why not always set keyspace_id = keyspace_Id_ ? Is there any case that you will create a TableInfo without passing a valid keyspace_id_ when using api_v2?

Make sense, this logic intends to use the original keyspace id if the JSON object parameter has a keyspace id field. But this might useless since there is no keyspace id in the upstream table schema.

dbms/src/Storages/Transaction/TiDB.h

dbms/src/Storages/Transaction/TMTStorages.h

dbms/src/TiDB/Schema/SchemaBuilder.h

dbms/src/TiDB/Schema/TiDBSchemaSyncer.h

ystaticy · 2023-02-16T08:55:20Z

dbms/src/TiDB/Schema/SchemaSyncService.cpp

@@ -40,35 +40,42 @@ SchemaSyncService::SchemaSyncService(DB::Context & context_)
    handle = background_pool.addTask(
        [&, this] {
            String stage;
+            auto keyspaces = context.getTMTContext().getStorages().getAllKeyspaces();


Does it contain NullspaceID?

Yes, it will contain Nullspace.

dbms/src/TiDB/Schema/SchemaSyncService.cpp

yongman · 2023-02-16T09:25:29Z

dbms/src/TiDB/Schema/SchemaSyncService.cpp

            }
            catch (const std::exception & e)
            {
-                LOG_ERROR(log, "{} failed by {}", stage, e.what());
+                LOG_ERROR(log, "[keyspace {}] {} failed by {}", cur_ks, stage, e.what());
            }
            return false;
        },


It's better make the task running interval be configurable.

yongman · 2023-02-21T02:32:46Z

dbms/src/Storages/GCManager.cpp

-    next_table_id = iter->first;
-    LOG_DEBUG(log, "End GC and next gc will start with table id: {}", next_table_id);
+
+    if (iter != storages.end())


This if statement always be true, this line check seems useless.

This statement will be false if there is no storage to GC, for example, the TiFlash instance just starts up and there is no schema syncing happening before it receives some r/w request. Otherwise, an undefined value will be logged.

dbms/src/Flash/Management/ManualCompact.cpp

dbms/src/Server/Server.cpp

dbms/src/Storages/StorageDisaggregated.cpp

dbms/src/Storages/Transaction/KeyspaceSnapshot.cpp

JaySon-Huang · 2023-03-09T05:07:18Z

dbms/src/Storages/Transaction/ProxyFFIStatusService.cpp

+        auto * log = &Poco::Logger::get("HandleHttpRequestSyncStatus");
+        LOG_DEBUG(log, "handling sync status request, path: {}, api_name: {}", path, api_name);
+
+        // Query schema: /keyspace/{keyspace_id}/table/{table_id}


do we keep the old query schema without keyspace?

Yes, the TiFlash replica status might be uncheckable in the rolling upgrade, we support the old schema.

dbms/src/Storages/Transaction/TMTStorages.h

dbms/src/TiDB/Schema/TiDBSchemaSyncer.h

JaySon-Huang

Mostly LGTM, please create the PRs to client-c and kvproto

dbms/src/TiDB/Schema/SchemaNameMapper.h

dbms/src/Server/Server.cpp

iosmanthus · 2023-03-09T12:47:02Z

@JaySon-Huang all comments have been addressed, PTAL. ❤️ The kvproto and client-c's PRs are listed in the PR description.

JaySon-Huang · 2023-03-09T14:07:28Z

/rebuild

JaySon-Huang · 2023-03-10T09:20:59Z

The client-c hash is not updated to 04d408143e5ceb01799ec642560c39dc1a0a373f

JaySon-Huang · 2023-03-10T09:22:26Z

why the tiflash-proxy hash is updated?

iosmanthus · 2023-03-10T09:24:50Z

The client-c hash is not updated to 04d408143e5ceb01799ec642560c39dc1a0a373f

This has been done.

JaySon-Huang

LGTM

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

JaySon-Huang · 2023-03-12T12:28:00Z

/merge

flowbehappy · 2023-03-12T14:11:10Z

/merge

JaySon-Huang · 2023-03-12T14:20:15Z

pending for pingcap/kvproto#1076

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

JaySon-Huang · 2023-03-12T14:29:23Z

/merge

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

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

JaySon-Huang · 2023-03-12T15:46:14Z

/merge

ti-chi-bot · 2023-03-12T15:46:16Z

@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 · 2023-03-12T15:46:17Z

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

Commit hash: a25cb09

iosmanthus · 2023-03-12T16:27:53Z

/merge

ti-chi-bot · 2023-03-12T16:27:54Z

@iosmanthus: 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 · 2023-03-12T16:27:55Z

@iosmanthus: /merge is only allowed for the committers, you can assign this pull request to the committer in list by filling /assign @committer in the comment to help merge this pull request.

In response to this:

/merge

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 added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 14, 2023

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 14, 2023

JaySon-Huang reviewed Feb 14, 2023

View reviewed changes

dbms/src/Interpreters/IDAsPathUpgrader.cpp Outdated Show resolved Hide resolved

This was referenced Feb 14, 2023

Support TiFlash replica in keyspace mode pingcap/tidb#41414

Closed

ddl: support creating tiflash replica in API V2 pingcap/tidb#41415

Merged

yongman reviewed Feb 15, 2023

View reviewed changes

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

JaySon-Huang reviewed Feb 16, 2023

View reviewed changes

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

ystaticy reviewed Feb 16, 2023

View reviewed changes

yongman reviewed Feb 16, 2023

View reviewed changes

dbms/src/TiDB/Schema/SchemaSyncService.cpp Outdated Show resolved Hide resolved

yongman reviewed Feb 16, 2023

View reviewed changes

JaySon-Huang mentioned this pull request Feb 20, 2023

*: Refine some logging level #6844

Merged

12 tasks

yongman reviewed Feb 21, 2023

View reviewed changes

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

JaySon-Huang mentioned this pull request Feb 27, 2023

The task in BackgroundProcessingPool is not run by fixed interval #6908

Closed

JaySon-Huang reviewed Mar 9, 2023

View reviewed changes

dbms/src/TiDB/Schema/SchemaNameMapper.h Outdated Show resolved Hide resolved

dbms/src/TiDB/Schema/SchemaNameMapper.h Outdated Show resolved Hide resolved

dbms/src/Server/Server.cpp Outdated Show resolved Hide resolved

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

JaySon-Huang approved these changes Mar 10, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 10, 2023

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 12, 2023

fix

0f154ee

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

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Mar 12, 2023

This comment was marked as outdated.

Sign in to view

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 12, 2023

Merge branch 'master' into keyspace

17561cd

This comment was marked as outdated.

Sign in to view

update compact

8432d5c

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

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Mar 12, 2023

This comment was marked as outdated.

Sign in to view

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 12, 2023

fix GenCreateTableStatement

94aff9b

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Mar 12, 2023

JaySon-Huang added 2 commits March 12, 2023 23:45

Fix ut

78cab27

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

Fix compile

a25cb09

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 Mar 12, 2023

ti-chi-bot merged commit 835ad98 into pingcap:master Mar 12, 2023

ystaticy mentioned this pull request Dec 6, 2023

Introduce Keyspace pingcap/tidb#40425

Open

23 tasks

support keyspace feature #6816

support keyspace feature #6816

Conversation

iosmanthus commented Feb 14, 2023 • edited by JaySon-Huang Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Feb 14, 2023 • edited Loading

sre-bot commented Feb 14, 2023 • 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

JaySon-Huang left a comment

Choose a reason for hiding this comment

iosmanthus commented Mar 9, 2023

JaySon-Huang commented Mar 9, 2023

JaySon-Huang commented Mar 10, 2023

JaySon-Huang commented Mar 10, 2023

iosmanthus commented Mar 10, 2023

JaySon-Huang left a comment

Choose a reason for hiding this comment

JaySon-Huang commented Mar 12, 2023

This comment was marked as outdated.

This comment was marked as outdated.

flowbehappy commented Mar 12, 2023

This comment was marked as outdated.

JaySon-Huang commented Mar 12, 2023

JaySon-Huang commented Mar 12, 2023

This comment was marked as outdated.

This comment was marked as outdated.

JaySon-Huang commented Mar 12, 2023

ti-chi-bot commented Mar 12, 2023

ti-chi-bot commented Mar 12, 2023

iosmanthus commented Mar 12, 2023

ti-chi-bot commented Mar 12, 2023

ti-chi-bot commented Mar 12, 2023

iosmanthus commented Feb 14, 2023 •

edited by JaySon-Huang

Loading

ti-chi-bot commented Feb 14, 2023 •

edited

Loading

sre-bot commented Feb 14, 2023 •

edited

Loading