Reapply "storage/copy-to-s3: emit empty file even if input is empty" #30844
Conversation
|
cc @pH14 @doy-materialize — lightly poking at this to get the catalog exporter work moving again. |
benesch
force-pushed
the
copy-to-s3-empty
branch
3 times, most recently
from
c8279d9 to
c8fc988
Compare
benesch
force-pushed
the
copy-to-s3-empty
branch
from
c8fc988 to
bdf7b1a
Compare
|
The second commit in this PR fixes the CI flakiness that caused the original revert. I've written up a more detailed diagnosis of the problem and the fix on the issue: https://github.com/MaterializeInc/database-issues/issues/8599#issuecomment-2558518257 |
|
I've been using this commit to run CI on the cloudtest testdrive script that was causing the flake 10 times: bdf7b1a IME this has reliably reproduced the flake in about 4/10 runs. |
| // partial data. | ||
| if shutdown_token.strong_count() == 0 { | ||
| // Wedge until the operator is dropped. | ||
| future::pending::<()>().await; |
There was a problem hiding this comment.
What is the mechanism that will drop the iterator? I think it could be the button AsyncOperatorBuilder::build returns, but we are not using that for this operator. Doesn't that mean we will keep this future alive forever?
There was a problem hiding this comment.
Responded on Slack, but whoops, thanks! Updated the approach here to use the async operator builder's buttons.
benesch
force-pushed
the
copy-to-s3-empty
branch
from
bdf7b1a to
c2f4ca2
Compare
benesch
force-pushed
the
copy-to-s3-empty
branch
4 times, most recently
from
01932d6 to
6da67dd
Compare
|
Whew, ok, test flakes tracked down. I had CI run "full testdrive in cloudtest" 10 times against this patchset and it passed all ten times. Details the fixes are in commit messages. |
src/aws-util/Cargo.toml
Outdated
| @@ -20,7 +20,7 @@ bytes = "1.3.0" | |||
| bytesize = "1.1.0" | |||
| http = "1.1.0" | |||
| hyper-tls = "0.5.0" | |||
| mz-ore = { path = "../ore", default-features = false } | |||
| mz-ore = { path = "../ore", default-features = true } | |||
There was a problem hiding this comment.
can we list the specific features required instead? just enabling default-features is going to break cloud, because it'll make aws-util start pulling in the workspace-hack crate
There was a problem hiding this comment.
Ah yeah this was just part of the revert commit. I'll just back this out. Doesn't seem to be required.
This reverts commit b1b2c28.
The COPY TO S3 sink must not flush files to S3 during shutdown, as those files may only be partially written. This can cause correctness issues when running copy operations on multiprocess replicas, as the lagging replica's copy sink will get dropped partway through. The solution is to properly convert the buttons returned by the async operators in the S3 sink's implementation into tokens that are dropped when the sink is dropped. This ensures that the oneshot sink is properly shut down when dropped, before it can write partial data to S3. Fix MaterializeInc/database-issues#8599.
A copy to operation involves some preflight checks: validating that the requested prefix of S3 is empty, and writing an INCOMPLETE sentinel file. Previously these checks were run as the first operation on every replica participating in the copy to operation. This was racy though: a lagging replica could write the INCOMPLETE sentinel file *after* the leading replica finished the copy and had thus *deleted* the INCOMPLETE sentinel file. Since the lagging replica's work is canceled once the leading replica's work is finished, this could result in a INCOMPLETE sentinel file that never got removed. This commit moves these preflight checks to the adapter, where they are performed exactly once before any replica is notified of the copy to operation. This ensures that the INCOMPLETE file is written at most once in an copy to operation, and in particular that lagging replicas can't cause the file to reappear afer the leading replica finishes the operation.
So that the adapter crate doesn't need to depend on the mz-storage-operators crate, which is verboten. This commit is pure code movement.
benesch
force-pushed
the
copy-to-s3-empty
branch
from
6da67dd to
95031e5
Compare
This reverts commit b1b2c28.
Fix https://github.com/MaterializeInc/database-issues/issues/8599.
Motivation
Tips for reviewer
Still TODO: track down test flake that caused this to get reverted in the first place.Test flake is solved! See commit messages for details.Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.