-
Notifications
You must be signed in to change notification settings - Fork 463
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
pageserver: do not read redundant timeline_layers
from IndexPart, so that we can remove it later
#4972
Conversation
timeline_layers is no longer used, no need to cross reference it with layer_metadata
1624 tests run: 1550 passed, 0 failed, 74 skipped (full report)The comment gets automatically updated with the latest test results
0657362 at 2023-08-21T11:10:48.836Z :recycle: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the two lists match, only one has values in addition. It's thus redundant and can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should bump the version (cannot add comment) because this is a meaningful change overall. It'll all contribute to us being able to spot if some old version is surprisingly still lurking around when doing cleanups.
No other issues.
Maybe worth noting in this PR the earlier commit that started requiring By the way, in theory, there is no guarantee that Control Plane has attached all tenants all the time. I guess it's fine though. |
That's important to keep in mind. I think right now we are still writing out the old format, we just don't read the field any more, basically allowing for |
This is not strictly necessary as the serialized format remains the same in practice, but will give us visibility of what IndexParts were written with the recently changed code in case of issues.
I checked that we didn't have any indices in the field with |
Problem
IndexPart contains two redundant lists of layer names: a set of the names, and then a map of name to metadata.
We already required that all the layers in
timeline_layers
are also inlayers_metadata
, ininitialize_with_current_remote_index_part
, so if there were any index_part.json files in the field that relied on these sets being different, they would already be broken.Summary of changes
timeline_layers
is made private and no longer read at runtime. It is still serialized, but not deserialized.disk_consistent_lsn
is also made private, as this field only exists for convenience of humans reading the serialized JSON.This prepares us to entirely remove
timeline_layers
in a future release, once this change is fully deployed, and therefore no pageservers are trying to read the field.Checklist before requesting a review
Checklist before merging