Skip to content
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

Control arrangement merging with default_exert_logic #23029

Merged
merged 3 commits into from
Nov 9, 2023

Conversation

teskje
Copy link
Contributor

@teskje teskje commented Nov 8, 2023

Add an alternative merging strategy for arrangements.

Up to this point, we could either not merge arrangements outside of ingesting updates, or always keep them in the most compacted form by specifying idle_merge_effort. Since TimelyDataflow/differential-dataflow#411 landed, Differential offers to replace the whole exertion logic, which we conditionally do with this PR. When the idle merge effort is 0, we honor an alternative setting of default_arrangement_exert_proprotionality that specifies which batches in an arrangement should be merged. Specifically, the proportionality determines when to merge the largest with the next largest batch.

Motivation

  • Fixes a known bug

Advanced MaterializeInc/database-issues#4918

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered.
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • This PR includes the following user-facing behavior changes:

@antiguru antiguru marked this pull request as ready for review November 9, 2023 20:01
@antiguru antiguru requested review from a team November 9, 2023 20:01
@antiguru antiguru requested review from a team as code owners November 9, 2023 20:01
Copy link

shepherdlybot bot commented Nov 9, 2023

This PR has higher risk. Make sure to carefully review the file hotspots. In addition to having this change reviewed, adequate tests should be considered and it may be useful to add observability and/or a feature flag. What's This?

Risk Score Probability Buggy File Hotspots
🔴 81 / 100 61% 2
Buggy File Hotspots:
File Percentile
../session/vars.rs 100
../src/coord.rs 99

@antiguru antiguru changed the title [poc] control arrangement merging with default_extert_logic Control arrangement merging with default_exert_logic Nov 9, 2023
Copy link
Contributor

@umanwizard umanwizard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea sounds fine, but I can't actually evaluate what this does without reading the internals of Differential

src/cluster/src/server.rs Outdated Show resolved Hide resolved
src/cluster/src/server.rs Show resolved Hide resolved
Signed-off-by: Moritz Hoffmann <[email protected]>
@antiguru antiguru merged commit e77aecd into MaterializeInc:main Nov 9, 2023
72 of 75 checks passed
@teskje teskje deleted the test-idle-merging branch November 10, 2023 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants