pageserver: refactor TenantId to TenantShardId in Tenant & Timeline #5957
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
jcsp
added
t/feature
2400 tests run: 2303 passed, 0 failed, 97 skipped (full report)Flaky tests (5)Postgres 14Code coverage (full report)
The comment gets automatically updated with the latest test results
02b8603 at 2023-11-29T15:03:48.860Z :recycle: |
jcsp
force-pushed
the
jcsp/sharding-tenant-ids
branch
from
31f613a to
ef28282
Compare
jcsp
force-pushed
the
jcsp/sharding-tenant-ids
branch
from
ef28282 to
80e6666
Compare
koivunej
reviewed
Nov 29, 2023
koivunej
reviewed
Nov 29, 2023
Yes indeed, but later: I intend to do that once all the TenantId locations are updated to be shard-aware (this PR is just the primary Tenant/Timeline path) |
jcsp
force-pushed
the
jcsp/sharding-tenant-ids
branch
from
80e6666 to
e6ec401
Compare
jcsp
added a commit
that referenced
this pull request
Nov 29, 2023
…5960) Precursor for #5957 ## Problem When DeletionList was written, TenantId/TimelineId didn't have human-friendly modes in their serde. #5335 added those, such that the helpers used in serialization of HashMaps are no longer necessary. ## Summary of changes - Add a unit test to ensure that this change isn't changing anything about the serialized form - Remove the serialization helpers for maps of Id
problame
approved these changes
Nov 29, 2023
There was a problem hiding this comment.
How did you ensure that in all the places we use Serialize, Display, Debug impls for TenantShardId, it's actually supposed to print the full TenantShardId and not just TenantId?
It wouldn't be noticable right now because we only have TenantShardId::unsharded. But, it could bite us at some later point.
For example, how did you ensure you caught all the #[instrument()] cases.
The debug_assert_span_contains... is useful for that.
Lastly I wonder whether our tracing spans should includethe monotonic version number of the sharding config (I assume such a thing exists somewhere outside of PS).
jcsp
force-pushed
the
jcsp/sharding-tenant-ids
branch
from
e6ec401 to
02b8603
Compare
This was referenced Dec 4, 2023
jcsp
added a commit
that referenced
this pull request
Dec 11, 2023
## Problem In #5957, the most essential types were updated to use TenantShardId rather than TenantId. That unblocked other work, but didn't fully enable running multiple shards from the same tenant on the same pageserver. ## Summary of changes - Use TenantShardId in page cache key for materialized pages - Update mgr.rs get_tenant() and list_tenants() functions to use a shard id, and update all callers. - Eliminate the exactly_one_or_none helper in mgr.rs and all code that used it - Convert timeline HTTP routes to use tenant_shard_id Note on page cache: ``` struct MaterializedPageHashKey { /// Why is this TenantShardId rather than TenantId? /// /// Usually, the materialized value of a page@lsn is identical on any shard in the same tenant. However, this /// this not the case for certain internally-generated pages (e.g. relation sizes). In future, we may make this /// key smaller by omitting the shard, if we ensure that reads to such pages always skip the cache, or are /// special-cased in some other way. tenant_shard_id: TenantShardId, timeline_id: TimelineId, key: Key, } ```
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.
(includes two preparatory commits from #5960)
Problem
To accommodate multiple shards in the same tenant on the same pageserver, we must include the full TenantShardId in local paths. That means that all code touching local storage needs to see the TenantShardId.
Summary of changes
tenant_id: TenantIdwithtenant_shard_id: TenantShardIdon Tenant, Timeline and RemoteTimelineClient.This doesn't update absolutely everything: things like PageCache, TaskMgr, WalRedo are still shard-naive. The purpose of this PR is to update the core types so that others code can be added/updated incrementally without churning the most central shared types.