fix: max_lsn_wal_lag broken in tenant conf
#4279
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
fails with E psycopg2.errors.InternalError_: Tenant a1fa9cc383e32ddafb73ff920de5f2e6 will not become active. Current state: Broken due to: Failed to parse config from file '/home/cs/src/neon/test_output/test_tenant_config[debug-pg14]/repo/tenants/a1fa9cc383e32ddafb73ff920de5f2e6/config' as pageserver config: configure option max_lsn_wal_lag is not a string. Backtrace: So, not even the assertions added are necessary. But let's have them anyway.
992 tests run: 951 passed, 0 failed, 41 skipped (full report)The comment gets automatically updated with the latest test results
9204bb3 at 2023-05-19T12:01:49.113Z :recycle: |
LizardWizzard
approved these changes
May 19, 2023
There was a problem hiding this comment.
LGTM
While looking at other integer-ish parameters there seems to be some inconsistency. checkpoint_distance is parsed using parse_toml_u64 and so many others. I tried to replace parse_toml_u64 with the same deserialize_from_item and test passed for me. So may make sense to unify those and possibly remove parse_toml_u64. Not requiring these changes to be made, up to you, feel free to merge as it is
koivunej
approved these changes
May 22, 2023
|
I'm waiting for e2e build failures to be resolved, see https://neondb.slack.com/archives/C03438W3FLZ/p1684747576440749 to |
5 tasks
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.
This patch fixes parsing of the
max_lsn_wal_lagtenant config item.We were incorrectly expecting a string before, but the type is a NonZeroU64.
So, when setting it in the config, the (updated) test case would fail with
So, not even the assertions added are necessary.
The test coverage for tenant config is rather thin in general.
For example, the
test_tenant_conf.pytest doesn't cover all the options.I'll add a new regression test as part of attach-time-tenant-conf PR #4255