#9275 refactor to batch compare and append for one shard in persist sink #32509
Conversation
This file contains hidden or 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
ptravers
changed the title
ptravers
force-pushed
the
9275-amortize-caa-persist-sink
branch
from
eb09eed to
8b969a6
Compare
ptravers
commented
May 27, 2025
ptravers
commented
May 27, 2025
src/persist-client/src/write.rs
Outdated
| @@ -484,6 +503,18 @@ where | |||
|
|
|||
| let lower = expected_upper.clone(); | |||
| let upper = new_upper; | |||
|
|
|||
| if let (Some(max_upper), Some(min_lower)) = (max_upper, min_lower) { | |||
| if max_upper.lt(&upper) || min_lower.gt(&lower) { | |||
There was a problem hiding this comment.
unclear that the min_lower check is valid but it does replicate the behaviour tested for here.
There was a problem hiding this comment.
removed the min_lower check as the kafka-rtr tests and others started failing. I am concerned that we are not really gathering the set of batches as a batch that would have been previously written.
EDIT: concern is that we are dropping batches that would have been written. I will add a counter or warn log for this issue to aid profiling.
ptravers
force-pushed
the
9275-amortize-caa-persist-sink
branch
from
9aac9cd to
dcf5ad9
Compare
ptravers
force-pushed
the
9275-amortize-caa-persist-sink
branch
from
dcf5ad9 to
3b5d25c
Compare
ptravers
force-pushed
the
9275-amortize-caa-persist-sink
branch
from
3b5d25c to
b18c329
Compare
ptravers
force-pushed
the
9275-amortize-caa-persist-sink
branch
from
5a7f5ff to
219dd42
Compare
ptravers
force-pushed
the
9275-amortize-caa-persist-sink
branch
from
219dd42 to
a44810f
Compare
ptravers
commented
May 29, 2025
| @@ -1360,6 +1358,8 @@ where | |||
| ); | |||
| anyhow::bail!("collection concurrently modified. Ingestion dataflow will be restarted"); | |||
| } | |||
|
|
|||
| break; | |||
There was a problem hiding this comment.
I am concerned that none of the above behaviour (everything in the Err(mismatch) arm) is ever actuated by a test.
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.
Motivation
https://github.com/MaterializeInc/database-issues/issues/9275
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.