compute: feed MV sink frontier from persist write handle #30662
+87
−68
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
This commit improves the trace logging in the MV sink v2 by adding a `sink_id` field.
This commit changes the way the MV sink produces updates to the shared sink frontier. Previously it would inspect the `persist` stream and forward that stream's frontier. The issue with this approach is that, in the face of ingestion spikes, the `persist` stream could fall behind, which would also make the MV sink's reported write frontier fall behind. This lead to a regression in the `CrossJoin` feature benchmark. In this benchmark, a large constant MV is created. As soon as the MV dataflow has produced its snapshot and has reported the empty frontier, the controller instructs the replica to drop it. If the reporting of the empty frontier is delayed because the MV sink is busy reading back the entire snapshot from persist, the controller's drop command also gets delayed. The result is that queries against the cluster get slowed down by the MV dataflow's processing, more than they would have been had the MV dataflow been dropped quickly. Attaching the shared write frontier to the `WriteHandle`-based frontier stream used by the `mint` operator avoids the issue.
This commit re-applies three minor changes that were the result of Dan's review comments on MaterializeInc#26745 but that somehow got lost in a rebase before that PR merged. * Replace `debug_assert`s with `assert`s. * Don't unnecessarily box operator tokens. * Add a clarifying comment about valid batch descriptions.
teskje
force-pushed
the
faster-mv-sink-frontier-reporting
branch
from
234d320
to
ec965a9
Compare
|
All nightlies failures are unrelated, afaict. In particular the upsert test failure also occurred in https://buildkite.com/materialize/nightly/builds/10582#_, which ran on a different PR. |
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.
This PR changes the way the MV sink produces updates to the shared sink frontier. Previously it would inspect the
persiststream and forward that stream's frontier. The issue with this approach is that, in the face of ingestion spikes, thepersiststream could fall behind, which would also make the MV sink's reported write frontier fall behind.This led to a regression in the
CrossJoinfeature benchmark. In this benchmark, a large constant MV is created. As soon as the MV dataflow has produced its snapshot and has reported the empty frontier, the controller instructs the replica to drop it. If the reporting of the empty frontier is delayed because the MV sink is busy reading back the entire snapshot from persist, the controller's drop command also gets delayed. The result is that queries against the cluster get slowed down by the MV dataflow's processing, more than they would have been had the MV dataflow been dropped quickly.Attaching the shared write frontier to the
WriteHandle-based frontier stream used by themintoperator avoids the issue.This PR also makes two minor changes to the MV sink (commits 1 and 3):
Motivation
Fixes https://github.com/MaterializeInc/database-issues/issues/8795
Tips for reviewer
Commits are meaningful and can be reviewed separately.
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.