Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(pageserver): add aux_file_v2 feature flag #7424

Closed
wants to merge 7 commits into from

Conversation

skyzh
Copy link
Member

@skyzh skyzh commented Apr 18, 2024

Problem

Added aux_file_v2 to the TimelineMetadata structure. The structure has not been touched for several months so I'm skeptical whether we can really add it there...

The flag cannot be reverted to false once being set. Once the tenant config has try_enable_aux_file_v2 set to true, the write path of AUX file will read this flag and set the timeline to have aux_file_v2 enabled. This flag will be persisted into the index part, so even if the flag got lost in the tenant config, the page server will still proceed with the v2 storage.

Summary of changes

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
  • If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

  • Do not forget to reformat commit message to not include the above checklist

Copy link

github-actions bot commented Apr 18, 2024

2754 tests run: 2633 passed, 1 failed, 120 skipped (full report)


Failures on Postgres 16

# Run all failed tests locally:
scripts/pytest -vv -n $(nproc) -k "test_ts_of_lsn_api[release-pg16]"
Flaky tests (1)

Postgres 15

  • test_vm_bit_clear_on_heap_lock: release

Test coverage report is not available

The comment gets automatically updated with the latest test results
37e25ab at 2024-04-24T18:30:47.141Z :recycle:

@skyzh skyzh mentioned this pull request Apr 22, 2024
24 tasks
Signed-off-by: Alex Chi Z <chi@neon.tech>

add tests

Signed-off-by: Alex Chi Z <chi@neon.tech>
Signed-off-by: Alex Chi Z <chi@neon.tech>
@skyzh
Copy link
Member Author

skyzh commented Apr 22, 2024

need to fix unit tests, e2e works.

@skyzh skyzh marked this pull request as ready for review April 22, 2024 18:19
@skyzh skyzh requested a review from a team as a code owner April 22, 2024 18:19
@skyzh skyzh requested a review from arpad-m April 22, 2024 18:19
Signed-off-by: Alex Chi Z <chi@neon.tech>
Signed-off-by: Alex Chi Z <chi@neon.tech>
Signed-off-by: Alex Chi Z <chi@neon.tech>
Signed-off-by: Alex Chi Z <chi@neon.tech>
Signed-off-by: Alex Chi Z <chi@neon.tech>
@arpad-m
Copy link
Member

arpad-m commented Apr 24, 2024

As said on slack, I think for the rollout we can just add it as a per-tenant config, no need to have it in timeline metadata. That would simplify this I think.

@skyzh
Copy link
Member Author

skyzh commented Apr 24, 2024

@arpad-m The timeline metadata is here to prevent the flag from being accidentally reverted, and also handle branching/history correctly. If the control plane / storage controller can guarantee that the flag won't be flipped, then it's fine to set it as a tenant config. (I can simply rename the current tenant config try_enable_aux_file_v2 to aux_file_v2 and drop all timeline metadata code if we decide to do so.)

@skyzh
Copy link
Member Author

skyzh commented Apr 24, 2024

I feel the only thing we are uncertain is whether we need to persist the aux_file_v2 state. Maybe I can submit a pull request with only tenant config change to unblock myself, and go back to this discussion later?

@skyzh
Copy link
Member Author

skyzh commented Apr 24, 2024

Opened #7505 instead. We can discuss the metadata change later.

@skyzh skyzh closed this Apr 25, 2024
skyzh added a commit that referenced this pull request Apr 26, 2024
Changing metadata format is not easy. This pull request adds a
tenant-level flag on whether to enable aux file v2. As long as we don't
roll this out to the user and guarantee our staging projects can persist
tenant config correctly, we can test the aux file v2 change with setting
this flag. Previous discussion at
#7424.

Signed-off-by: Alex Chi Z <chi@neon.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants