-
Notifications
You must be signed in to change notification settings - Fork 393
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
fix: EphmeralFiles can outlive their Timeline, new gate version #8332
Conversation
this took a while to find..
The two-gate idea comes from @jcsp following the larger restructuring on #8229. Compared to the original solution where we track whether LayerManager is open or closed (shutdown), this version:
|
Will push the doc fix after regress tests. |
3042 tests run: 2926 passed, 1 failed, 115 skipped (full report)Failures on Postgres 14
Test coverage report is not availableThe comment gets automatically updated with the latest test results
607c044 at 2024-07-10T10:50:17.257Z :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.
now instead of type system we get the same safety from the runtime state of the two gates
I don't get how we get any of the safety I asked for in #8229 (comment)
Earlier comment:
The two gates is the answer:
I couldn't spot any dire implementation error on my part, so could you please be more specific in case your question was not of general nature...? |
I think we just disagree on whether we need robustness in depth or not. |
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.
Ok, Joonas and I had a call and he explained this PR to me.
It's a clever idea with few LoC but I would much prefer the robustness-in-depth that we gain from an explicitly tracked shutdown state.
See also my comment here: #8229 (comment)
Per Christians notes above, closing this down. Risk of no longer reading a should-be-present in-memory layer could cause us a lot of corruption. |
Ephemeral files cleanup on drop but did not delay shutdown, leading to problems with restarting the tenant. The solution is:
LayerManager::ephmeral_files
which is closed after all Timeline operations are closedFixes: #7830