ScheduledMerges: fix a bug in maxPhysicalDebt and invariant
#763
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
jorisdral
requested review from
dcoutts,
mheinzel,
recursion-ninja and
wenkokke
as code owners
This should help get a sense of *why* the assertion is failing
jorisdral
force-pushed
the
jdral/issue755
branch
from
8382914 to
2b86029
Compare
The prototype had a bug in the calculation of `maxPhysicalDebt` where it was not considering that incoming runs that are inputs to a merge can be overfull. Runs can be overfull because we hold back runs that are underfull. However, runs should only be overfull in the sense that they are too large for their current level. They can't be overfull with respect to the next level too. This same bug occurred in `invariant`, where a similar change was made to consider overfull runs properly. Some other minor changes to `invariant` were made to make the checked invariant a little more precise.
jorisdral
force-pushed
the
jdral/issue755
branch
from
2b86029 to
184809a
Compare
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.
Resolves #755
The prototype had a bug in the calculation of
maxPhysicalDebtwhere it was notconsidering that incoming runs that are inputs to a merge can be overfull. Runs
can be overfull because we hold back runs that are underfull. However, runs
should only be overfull in the sense that they are too large for their current
level. They can't be overfull with respect to the next level too.
This same bug occurred in
invariant, where a similar change was made toconsider overfull runs properly.
Some other minor changes to
invariantwere made to make the checked invarianta little more precise.
Note that the assertion failures only started appearing on CI runs after #716 and #717 were merged. I realised that it's a relatively rare scenario to create underfull and overfull runs, but less so if the size ratio and write buffer size are small. That's probably why we only just now started seeing the assertion failures.