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

[no-release-notes] go/store/nbs: NomsBlockStore.UpdateManifest{,WithAppendix} pre-open table files before the manifest updates. #8903

Merged
merged 2 commits into from
Feb 28, 2025

Conversation

reltuk
Copy link
Contributor

@reltuk reltuk commented Feb 28, 2025

This brings them in line with AddTableFilesToManifest.

UpdateManifest{,WithAppendix} are used by some implementations of dolt.services.remotesapi.v1alpha1.ChunkStoreService. Opening the table files is only less efficient if updateManifestAddFiles encounters an error, but it is safer because it ensures the table files are able to be opened before the added to the manifest itself. We do not expect updateManifestAddFiles to encounter errors - its only error condition is an I/O error on updating the manifest itself, or, previously, errors on opening the newly added table files.

…ppendix} pre-open table files before the manifest updates.

This brings them in line with AddTableFilesToManifest.

UpdateManifest{,WithAppendix} are used by some implementations of
dolt.services.remotesapi.v1alpha1.ChunkStoreService. Opening the table files is
only less efficient if updateManifestAddFiles encounters an error, but it is
safer because it ensures the table files are able to be opened before the added
to the manifest itself. We do not expect updateManifestAddFiles to encounter
errors - its only error condition is an I/O error on updating the manifest
itself, or, previously, errors on opening the newly added table files.
@reltuk reltuk requested a review from macneale4 February 28, 2025 00:59
Copy link
Contributor

@macneale4 macneale4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you can delete checkAllManifestUpdatesExist

@coffeegoddd
Copy link
Contributor

@reltuk DOLT

comparing_percentages
100.000000 to 100.000000
version result total
71d166f ok 5937457
version total_tests
71d166f 5937457
correctness_percentage
100.0

@macneale4 macneale4 merged commit de44965 into main Feb 28, 2025
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants