refactor: system tables in new region server #2344
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## migrate-region-server #2344 +/- ##
========================================================
Coverage ? 77.54%
========================================================
Files ? 724
Lines ? 115145
Branches ? 0
========================================================
Hits ? 89292
Misses ? 25853
Partials ? 0 |
|
There are some conflicts. |
MichaelScofield
force-pushed
the
refactor/system-catalog-tables
branch
from
0e82153 to
c918887
Compare
WenyXu
approved these changes
Sep 12, 2023
Merged
via the queue into
GreptimeTeam:migrate-region-server
with commit Sep 12, 2023
5f7d118
11 checks passed
waynexia
reviewed
Sep 12, 2023
Comment on lines
+54
to
+81
| // There are two sources for finding a table: the `local_catalog_manager` and the | ||
| // `table_metadata_manager`. | ||
| // | ||
| // The `local_catalog_manager` is for storing tables that are often transparent, not saving any | ||
| // real data. For example, our system tables, the `numbers` table and the "information_schema" | ||
| // table. | ||
| // | ||
| // The `table_metadata_manager`, on the other hand, is for storing tables that are created by users, | ||
| // obviously. | ||
| // | ||
| // For now, separating the two makes the code simpler, at least in the retrieval site. Now we have | ||
| // `numbers` and `information_schema` system tables. Both have their special implementations. If we | ||
| // put them with other ordinary tables that are created by users, we need to check the table name | ||
| // to decide which `TableRef` to return. Like this: | ||
| // | ||
| // ```rust | ||
| // match table_name { | ||
| // "numbers" => ... // return NumbersTable impl | ||
| // "information_schema" => ... // return InformationSchemaTable impl | ||
| // _ => .. // return DistTable impl | ||
| // } | ||
| // ``` | ||
| // | ||
| // On the other hand, because we use `MemoryCatalogManager` for system tables, we can easily store | ||
| // and retrieve the concrete implementation of the system tables by their names, no more "if-else"s. | ||
| // | ||
| // However, if the system table is designed to have more features in the future, we may revisit | ||
| // the implementation here. |
There was a problem hiding this comment.
- those are not doc comment
- add
,ignoreto the block language, otherwise it will become an uncompileable doc test
There was a problem hiding this comment.
intentionally not made them doc comments because they are not relevant to how to use this FrontendCatalogManager
There was a problem hiding this comment.
not real tables that will be persisted in the kv backend which the TableMatadataManager in the FrontendCatalogManager proxied to
waynexia
pushed a commit
that referenced
this pull request
Sep 12, 2023
refactor: inverse the dependency between system tables and catalog manager
waynexia
pushed a commit
that referenced
this pull request
Sep 12, 2023
refactor: inverse the dependency between system tables and catalog manager
waynexia
pushed a commit
that referenced
this pull request
Sep 12, 2023
refactor: inverse the dependency between system tables and catalog manager
WenyXu
pushed a commit
to WenyXu/greptimedb
that referenced
this pull request
Sep 13, 2023
refactor: inverse the dependency between system tables and catalog manager
paomian
pushed a commit
to paomian/greptimedb
that referenced
this pull request
Oct 19, 2023
refactor: inverse the dependency between system tables and catalog manager
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I hereby agree to the terms of the GreptimeDB CLA
What's changed and what's your intention?
This PR mainly refactor the dependency between system tables and catalog manager. Now system tables (numbers table and information schema table) will be injected to a local memory catalog in FrontendCatalogManager (other ordinary user tables are initialized from kv backend).
I'm trying to get rid of LocalCatalogManager. Will unify catalog managers to FrontendCatalogManager afterwards.
Checklist
Refer to a related PR or issue link (optional)