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

ddata ReadMajorityPlus and WriteMajorityPlus #5146

Merged
merged 6 commits into from Aug 2, 2021

Conversation

zbynek001
Copy link
Contributor

New ReadMajorityPlus and WriteMajorityPlus
Possibility to prefer oldest in ddata writes and reads
(partially migrated from akka/akka#28895)

@Aaronontheweb Aaronontheweb self-requested a review July 19, 2021 14:50
@Aaronontheweb Aaronontheweb added this to the 1.4.22 milestone Jul 19, 2021
Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

Overall, looks good to me - left you some questions though @zbynek001

ExpectTerminated(replicator, TimeSpan.FromSeconds(10));
var terminated1 = ExpectMsg<Terminated>(TimeSpan.FromSeconds(10));
var terminated2 = ExpectMsg<Terminated>(TimeSpan.FromSeconds(10));
ImmutableHashSet.Create(terminated1.ActorRef, terminated2.ActorRef).Should().BeEquivalentTo(durableStore, replicator);
Copy link
Member

Choose a reason for hiding this comment

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

Nice, that should make this test less racy

@@ -85,7 +85,7 @@ public WriteAckAdapter(IActorRef replica)
private readonly Address _nodeB = new Address("akka.tcp", "Sys", "b", 2552);
private readonly Address _nodeC = new Address("akka.tcp", "Sys", "c", 2552);
private readonly Address _nodeD = new Address("akka.tcp", "Sys", "d", 2552);
private readonly IImmutableSet<Address> _nodes;
private readonly IImmutableList<Address> _nodes;
Copy link
Member

Choose a reason for hiding this comment

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

Why the change from set to list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because the base class ReadWriteAggregator changes of nodes from IImmutableSet to IImmutableList to keep the order of nodes by oldest for the PreferOldest scenario.

Copy link
Member

Choose a reason for hiding this comment

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

Got it, thank you

var n = Nodes.Count + 1;
var r = CalculateMajority(read.MinCapacity, n, read.Additional);
Log.Debug("ReadMajorityPlus [{0}] [{1}] of [{2}].", _key, r, n);
return n - r;
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

// When RequiresCausalDeliveryOfDeltas (shuffle=false) use deterministic order to so that sequence numbers
// of subsequent updates are in sync on the destination nodes.
// The order is also kept when prefer-oldest is enabled.
var orderedNodes = Shuffle ?
Copy link
Member

Choose a reason for hiding this comment

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

The comment above makes sense - thank you


return Math.Max(minCapacity, numberOfNodes / 2 + 1);
var majority = numberOfNodes / 2 + 1;
return Math.Min(numberOfNodes, Math.Max(majority + additional, minCapacity));
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

{
proto.ConsistencyAdditional = rmp.Additional;
proto.HasConsistencyAdditional = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

Log.Debug("WriteMajorityPlus [{0}] [{1}] of [{2}].", _key, w, n);
return n - w;
}
case WriteLocal _: throw new ArgumentException("WriteAggregator does not support WriteLocal");
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

/// <see cref="WriteMajority"/> but with the given number of <see cref="Additional"/> nodes added to the majority count. At most
/// all nodes.
/// </summary>
public sealed class WriteMajorityPlus : IWriteConsistency, IEquatable<WriteMajorityPlus>
Copy link
Member

Choose a reason for hiding this comment

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

LGTM


# Update and Get operations are sent to oldest nodes first.
# This is useful together with Cluster Singleton, which is running on oldest nodes.
prefer-oldest = off
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@@ -17,6 +17,7 @@ message Get {
int32 consistencyMinCap = 5;
bool hasConsistencyAdditional = 6;
int32 consistencyAdditional = 7;
bool hasConsistencyMinCap = 8;
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@Aaronontheweb Aaronontheweb 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 044622f into akkadotnet:dev Aug 2, 2021
@zbynek001 zbynek001 deleted the ddata-majority-plus branch August 3, 2021 13:49
This was referenced Aug 5, 2021
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.

None yet

2 participants