fix(pageserver): make barrier waiting for deletion queue #9796
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
skyzh
force-pushed
the
skyzh/fix-barrier
branch
from
e6e5b27 to
aa2649c
Compare
arpad-m
reviewed
Nov 18, 2024
| @@ -1242,19 +1246,20 @@ impl RemoteTimelineClient { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| pub(crate) fn schedule_barrier(self: &Arc<Self>) -> anyhow::Result<()> { | |||
| pub(crate) fn schedule_barrier(self: &Arc<Self>, initial_barrier: bool) -> anyhow::Result<()> { | |||
There was a problem hiding this comment.
it would be better to have schedule_barrier_initial instead of adding a boolean parameter everywhere.
| ) | ||
| .await | ||
| .map_err(|e| anyhow::anyhow!(e)), | ||
| // Barrier flushes up the deletion queue. Usually, we don't wait until deletion |
There was a problem hiding this comment.
Suggested change
| // Barrier flushes up the deletion queue. Usually, we don't wait until deletion | |
| // Barrier flushes the deletion queue. Usually, we don't wait until deletion |
5544 tests run: 5210 passed, 108 failed, 226 skipped (full report)Failures on Postgres 17
Failures on Postgres 16
Failures on Postgres 15
Failures on Postgres 14
Flaky tests (2)Postgres 15
Test coverage report is not availableThe comment gets automatically updated with the latest test results
499105d at 2024-11-21T20:06:24.118Z :recycle: |
|
Removing myself from review, Arpad has more context |
|
test failure around Given the the behavior of a barrier is to trigger deletion, this makes me wonder (1) is our current implementation correct? why we will delete files before preload completes? (2) is this patch correct? i start doubting whether it's a good idea to have such waiting for deletion queue semantics for the initial barrier... |
|
|
Signed-off-by: Alex Chi Z <[email protected]>
Signed-off-by: Alex Chi Z <[email protected]>
Signed-off-by: Alex Chi Z <[email protected]>
Signed-off-by: Alex Chi Z <[email protected]>
skyzh
force-pushed
the
skyzh/fix-barrier
branch
from
ec9fcbd to
cdde254
Compare
jcsp
reviewed
Nov 20, 2024
| /// The boolean value indicates whether the barrier is an initial barrier scheduled | ||
| /// at timeline load -- if yes, we will need to wait for all deletions to be completed | ||
| /// before the next upload. | ||
| Barrier(tokio::sync::watch::Sender<()>, bool), |
There was a problem hiding this comment.
Let's use a descriptive enum rather than a bool here, to make it easier to read & harder to typo
jcsp
approved these changes
Nov 20, 2024
There was a problem hiding this comment.
This PR convinces me that:
A) this change improves safety 👍
B) we should later address the underlying complexity somehow, perhaps by modifying the ancestor detach to include a generation increment (the request flows through the storage controller so this is possible)
Signed-off-by: Alex Chi Z <[email protected]>
Signed-off-by: Alex Chi Z <[email protected]>
Signed-off-by: Alex Chi Z <[email protected]>
|
d710f00 changed the emergency mode behavior not to wait for uploads |
Signed-off-by: Alex Chi Z <[email protected]>
|
the new patch #9844 to fully address the problem in the remote client |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Follow up of #9682, that patch didn't fully address the problem: what if shutdown fails due to whatever reason and then we reattach the tenant? Then we will still remove the future layer. The underlying problem is that the fix for #5878 gets voided because of the generation optimizations. We should always wait for deletions for the barrier.
Summary of changes