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: generational_chunk_store.go: In GCMode_Full, also take dependencies on chunks read from OldGen. #8796

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

reltuk
Copy link
Contributor

@reltuk reltuk commented Jan 28, 2025

No description provided.

@reltuk reltuk requested a review from max-hoffman January 28, 2025 20:03
@reltuk reltuk changed the title go/store/nbs: generational_chunk_store.go: In GCMode_Full, also take dependencies on chunks read from OldGen. [no-release-notes] go/store/nbs: generational_chunk_store.go: In GCMode_Full, also take dependencies on chunks read from OldGen. Jan 28, 2025
@coffeegoddd
Copy link
Contributor

@reltuk DOLT

comparing_percentages
100.000000 to 100.000000
version result total
1b59420 ok 5937457
version total_tests
1b59420 5937457
correctness_percentage
100.0

Copy link
Contributor

@max-hoffman max-hoffman left a comment

Choose a reason for hiding this comment

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

LGTM. Just so I'm following, the case where this would be useful is something like, the rootvalue does not reach certain chunks in oldgen, but there is a commit in progress who's new rootvalue does ref an oldgen chunk, and we want to include that refs oldgen closure in the new table file rather that GC'ing it and leaving the new root hanging. And this only matters for a full GC because that's the only case the old oldgen tablefiles are removed.

@reltuk
Copy link
Contributor Author

reltuk commented Jan 30, 2025

LGTM. Just so I'm following, the case where this would be useful is something like, the rootvalue does not reach certain chunks in oldgen, but there is a commit in progress who's new rootvalue does ref an oldgen chunk, and we want to include that refs oldgen closure in the new table file rather that GC'ing it and leaving the new root hanging. And this only matters for a full GC because that's the only case the old oldgen tablefiles are removed.

Totally correct understanding. The theory is some inflight work is going to take a dep on oldgen, but the roots we saw when we started the GC don't reach those chunks.

@reltuk reltuk merged commit 3dd50f5 into main Jan 30, 2025
34 of 35 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.

None yet

3 participants