Fix row ordering flakiness when using clear APIs #3288
Merged
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
teh-cmc
added
🪳 bug
teh-cmc
commented
Sep 12, 2023
teh-cmc
force-pushed
the
cmc/clear_ordering_flakiness
branch
from
29537b4 to
7faa657
Compare
3 tasks
emilk
approved these changes
Sep 12, 2023
teh-cmc
added a commit
that referenced
this pull request
Sep 13, 2023
**Commit by commit** This only adds unit tests and API examples: no need for dedicated roundtrip tests since all possible cases are already covered by the API example roundtrips. I want a better constructor interface for Python, but this requires #3268. Requires #3288, otherwise the roundtrip tests are flaky.
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.
The clear APIs insert multiple rows worth of data (1 per component) that all re-use the same
RowId(theRowIdthat was used to do the log_clear() call itself).This leads to a flaky ordering of these rows internally, as there is no way to tie-break between them.
It is also less performant than it should be as all of this data could and should live on the same row.
The problem is accentuated when doing recursive clears, since in that case the row ID is not only shared between multiple rows of the same entity, but also multiple rows across multiple entities: even more ordering flakiness.
This flaky ordering then leaks through the public APIs (e.g. range queries) and, importantly, through roundtrip tests.
This is how I first encoutered this issue: #3023 is blocked by this because the roundtrip tests for recursive clears are flaky. This PR fixes that.
Unfortunately we don't have a test suite in place for clear APIs (AFAIK), and I'm not even sure what the expected behavior is in all cases, so I cannot write that test suite for now. I've opened an issue for this:
In the meantime, the roundtrip tests introduced in #3023 will act as a regression test at the very least.
What
Checklist