-
Notifications
You must be signed in to change notification settings - Fork 487
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 write path #7491
Conversation
Signed-off-by: Alex Chi Z <chi@neon.tech>
2766 tests run: 2646 passed, 0 failed, 120 skipped (full report)Flaky tests (2)Postgres 16
Postgres 14
Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
82327a8 at 2024-04-24T20:26:31.239Z :recycle: |
@@ -1392,6 +1392,37 @@ impl<'a> DatadirModification<'a> { | |||
content: &[u8], | |||
ctx: &RequestContext, | |||
) -> anyhow::Result<()> { | |||
const AUX_FILES_V2: bool = false; // disable for now until we settle down the feature flag for aux file v2 |
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.
Hm. I don't feel like we should merge dead code. IMO there should be at least a unit test exercising it.
I'll need to finish the read path #7468 before I could merge this pull request. That one doesn't seem simple because there will need to be some significant changes in image layer creation. |
Signed-off-by: Alex Chi Z <chi@neon.tech>
Signed-off-by: Alex Chi Z <chi@neon.tech>
added an encode/decode test, need to wait #7505 to put the logic behind a proper feature flag. The aux file write path can only be tested when the basebackup read path is implemented, so I'd rather leave it untested in this pull request. |
Can you not test with just |
I assume we will need a compute node to write AUX files into the page server, and in that case, if we want to test read a single file without read path functionalities implemented (i.e., get single file, or generate basebackup), the test (in Python regression test) will need to compute the hash of the file and encode the value in order to do the comparison, or hardcode the key/value, which duplicates the implementation in the page server side. Maybe I will have a single pull request with both read+write path for AUX file later so that we can merge some code with good test coverage. For the generic case, I have #7468 that tests writing arbitrary keys and read them. |
Problem
#7462
Summary of changes
Add the write path of aux file v2. We first retrieve the value of the corresponding hash key. If the key exists (which indicates a hash collision), modify the value according to the type of file operation (i.e., update or insert). One value can store multiple files, though the chance of storing multiple files is low.
Checklist before requesting a review
Checklist before merging