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

fix: properly update branch version #61

Merged
merged 2 commits into from
Oct 30, 2024
Merged

fix: properly update branch version #61

merged 2 commits into from
Oct 30, 2024

Conversation

alemidev
Copy link
Member

basically we used to merge and update local version while generating deltas, but it shouldnt happen (we added ack for that)

this changes how hash works however: it now reflects the state of the buffer before applying this change, not after. i think its a bit less useful but i dont see other ways

@alemidev alemidev added bug Something isn't working as intended needs research This issue needs further research core This issue is related to the core Rust library labels Oct 27, 2024
@ghyatzo
Copy link
Member

ghyatzo commented Oct 27, 2024

I will have to think this through.

If I recall correctly, we used to incrementally add changes, since we are grabbing from and iterator of changes. It might be more than one.

So the step_ver was the version after one step, and it might not be at the frontier. We keep this so that the next time we can get the next change in the sequence correctly.

I will have to go back in time a bit and remember how it was before, and how it changed when ack was added to figure out what went wrong though.

I'd like to err on the side of caution and not merge this until we are (maybe it's just me?) more sure this is the actual fix.

@zaaarf
Copy link
Member

zaaarf commented Oct 27, 2024

I don't think it matters all that much whether hash represents before or after. I mean, sure, obviously it requires changes in the plugin impl, but it's not like we set hash every time anyway. Instead of being caught in the current iteration, a desync will be caught in the one after that. I don't think it necessarily makes hash less useful.

The code/logic looks fine to me, but I'll leave it to @ghyatzo's judgement to assess whether it's the change that fixes it since he's the resident CRDT connoisseur.

Just one thing: why'd you change Default::default()to specify the impl name? Is this going to be consistent throughout the codebase now?

Copy link
Member

@ghyatzo ghyatzo left a comment

Choose a reason for hiding this comment

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

I'm as convinced as I can by just reading code. Looks good, guess we need to test it now.

@ghyatzo ghyatzo merged commit 9d458f6 into dev Oct 30, 2024
64 checks passed
@ghyatzo ghyatzo deleted the fix/version-acking branch October 30, 2024 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as intended core This issue is related to the core Rust library needs research This issue needs further research
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants