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 sharding tolerant reader #5976

Merged

Conversation

Aaronontheweb
Copy link
Member

@Aaronontheweb Aaronontheweb commented May 31, 2022

Fixes #5604
Reverts #5967

Changes

De-duplicate only exact ShardHomeAllocated duplicate data, makes the changes to the PersistentShardCoordinator safer and more narrow in scope than #5967

Checklist

For significant changes, please ensure that the following have been completed (delete if not relevant):

case ShardHomeAllocated homeAllocated:
// if we already have identical ShardHomeAllocated data, skip processing it
// addresses https://github.com/akkadotnet/akka.net/issues/5604
if (CurrentState.Shards.TryGetValue(homeAllocated.Shard, out var currentShardRegion)
Copy link
Member Author

Choose a reason for hiding this comment

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

We had an identical duplicate message. Ignore it and continue.

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be much safer - cc @ismaelhamed

Copy link
Member

Choose a reason for hiding this comment

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

Yes. This looks more like what we've done in the past (basically delete newest ShardHomeAllocated events).

I think this is due to having two active clusters at the same time, probably because of a network partition during shutdown of some of the nodes.

There's another take of this one in which you get shards that have not being allocated yet: akka/akka#17955

But it's been ages since I've seen any of those, to be honest.

Copy link
Contributor

@Arkatufus Arkatufus left a comment

Choose a reason for hiding this comment

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

LGTM, I'll make a backport after this is merged.

@Aaronontheweb
Copy link
Member Author

LGTM, I'll make a backport after this is merged.

Hopefully @ismaelhamed gets back to us in a day or two with his comments - I don't want to merge a change into this code over his objections even though we're "on the clock" to get some fixes out to v1.4 soon.

Copy link
Member

@ismaelhamed ismaelhamed left a comment

Choose a reason for hiding this comment

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

LGTM

@Aaronontheweb Aaronontheweb merged commit 36bc254 into akkadotnet:v1.4 Jun 1, 2022
@Aaronontheweb Aaronontheweb deleted the fix-sharding-tolerant-reader branch June 1, 2022 11:45
Arkatufus added a commit to Arkatufus/akka.net that referenced this pull request Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants