Fix #238: Non converging operations with unordered deletes. Migration to fix existing datastores. #241
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
hsanjuan
force-pushed
the
fix/238-non-converging
branch
2 times, most recently
from
84d8f8a
to
279fb69
Compare
|
@dozyio I wonder if you are able to have a look |
|
Being playing around a bit with this PR and it seems good! Puts and deletes converging as expected. Tested the migrations a little bit and seems ok. Thanks for fixing the bug! |
dozyio
reviewed
Nov 10, 2024
dozyio
approved these changes
Nov 10, 2024
hsanjuan
force-pushed
the
fix/238-non-converging
branch
from
279fb69
to
5d15cf8
Compare
hsanjuan
commented
Nov 11, 2024
The problem: a replica may tombstone a value and then receive a new write for that value that had happened BEFORE the tombstone from a different replica. The final value/priority pair should be set to the value/priority that was not tombstoned. However we did not do that. We knew the key was not tombstoned, but the value returned corresponded to an update that was tombstoned. The solution: every time a tombstone arrives, we need to look for the "best value/priority", that is, we need to make sure that among all the key values that we have, we set the best one that was not tombstoned according to the CRDT rules (highest priority or lexicographical sorting when equal). The consecuences: this makes tombstoning a more expensive operation but it also allows us to remove value/priority altogether when all the values have been tombstoned. As such, we don't need to check if a value has been tombstoned anymore when doing Gets/List, before returning the element. That saves lookups and that also means we no longer need to bloom filter, which was supposed to speed up this operation. In general, datastore which mostly add data will be better afterwards.
hsanjuan
force-pushed
the
fix/238-non-converging
branch
from
5d15cf8
to
ca44680
Compare
The fix for #238 does not mean that everything is fine. We will have databases which have the wrong value/priority sets written and this would only fix itself on new writes or deletes to the same key. So we are unfortunately forced to manually fix it on start. For this we introduce a data migration. During a fresh start, we will then find all the keys affected by tombstones and loop them, finding the best value for them (the correct one) and fixing them. Once done we record that we are on version=1 and don't run this again. Future fuckups can be fixed with other migrations.
hsanjuan
force-pushed
the
fix/238-non-converging
branch
from
ca44680
to
7f73711
Compare
|
Thanks very much @dozyio for the review and for finding the bug! |
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 is made of two commits, one fixes the problem with the wrong values being set for elements that are partially tombstoned. The second adds a migration to fix potentially wrong values, but also to ensure that we return the right thing now that we don't check for tombstoning anymore on Get(). Fixes #238 .