refactor: attach: use create_tenant_files + schedule_local_tenant_processing #4235
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
…cessing With this patch, the attach handler now follows the same pattern as tenant create with regards to instantiation of the new tenant: 1. Prepare on-disk state using `create_tenant_files`. 2. Use the same code path as pageserver startup to load it into memory and start background loops (`schedule_local_tenant_processing`). It's a bit sad we can't use the `PageServerConfig::tenant_attaching_mark_file_path` method inside `create_tenant_files` because it operates in a temporary directory. However, it's a small price to pay for the gained simplicity. During implementation, I noticed that we don't handle failures post `create_tenant_files` well. I left TODO comments in the code linking to the issue that I created for this [^1]. Also, I'll dedupe the spawn_load and spawn_attach code in a future commit. [^1]: #4233
992 tests run: 944 passed, 0 failed, 48 skipped (full report for 5e0c147)Flaky testsPostgreSQL 15 (release build)PostgreSQL 15 (debug build)PostgreSQL 14 (release build)PostgreSQL 14 (debug build)The comment gets automatically updated with the latest test results ♻️
|
And make error message wording fit both create & attach use case.
problame
requested review from
save-buffer and
skyzh
and removed request for
a team and
save-buffer
problame
changed the title
koivunej
reviewed
May 16, 2023
shanyp
reviewed
May 16, 2023
This was referenced May 16, 2023
skyzh
approved these changes
May 16, 2023
problame
added a commit
that referenced
this pull request
May 24, 2023
This PR adds support for supplying the tenant config upon /attach. Before this change, when relocating a tenant using `/detach` and `/attach`, the tenant config after `/attach` would be the default config from `pageserver.toml`. That is undesirable for settings such as the PITR-interval: if the tenant's config on the source was `30 days` and the default config on the attach-side is `7 days`, then the first GC run would eradicate 23 days worth of PITR capability. The API change is backwards-compatible: if the body is empty, we continue to use the default config. We'll remove that capability as soon as the cloud.git code is updated to use attach-time tenant config (#4282 keeps track of this). unblocks neondatabase/cloud#5092 fixes #1555 part of #886 (Tenant Relocation) Implementation ============== The preliminary PRs for this work were (most-recent to least-recent) * #4279 * #4267 * #4252 * #4235
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.
With this patch, the attach handler now follows the same pattern as tenant create with regards to instantiation of the new tenant:
create_tenant_files.schedule_local_tenant_processing).It's a bit sad we can't use the
PageServerConfig::tenant_attaching_mark_file_pathmethod insidecreate_tenant_filesbecause it operates in a temporary directory. However, it's a small price to pay for the gained simplicity.During implementation, I noticed that we don't handle failures post
create_tenant_fileswell. I left TODO comments in the code linking to the issue that I created for this 1.Also, I'll dedupe the spawn_load and spawn_attach code in a future commit.
refs #1555
part of #886 (Tenant Relocation)
Footnotes
https://github.com/neondatabase/neon/issues/4233 ↩