-
Notifications
You must be signed in to change notification settings - Fork 1k
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
SBR fix & update #5147
SBR fix & update #5147
Conversation
Might take me a bit longer to get to reviewing this one, but I will do my best! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
using Akka.Event; | ||
using Akka.TestKit; | ||
using Akka.TestKit.Xunit2; | ||
using Akka.Util; | ||
|
||
namespace Akka.Cluster.Tools.Tests | ||
namespace Akka.Coordination.Tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the changes in this PR are due to moving this file around, so this PR is actually simpler than it appears.
public LeaseMajoritySettings(string leaseImplementation, System.TimeSpan acquireLeaseDelayForMinority, string role, string leaseName) { } | ||
public LeaseMajoritySettings(string leaseImplementation, System.TimeSpan acquireLeaseDelayForMinority, System.TimeSpan releaseAfter, string role, string leaseName) { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
} | ||
|
||
protected DownAllIndirectlyConnected5NodeSpec(DownAllIndirectlyConnected5NodeSpecConfig config) | ||
: base(config, typeof(DownAllIndirectlyConnected5NodeSpec)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MNTR constructors and config all look fine
{ | ||
AwaitAssert(() => | ||
{ | ||
cluster.State.Members.Select(i => i.Address).Should().BeEquivalentTo(Node(_config.Node1).Address); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So all of the nodes that have at least one broken connection are downed...
{ | ||
cluster.State.Unreachable.Select(i => i.Address).Should().BeEquivalentTo(new[] { _config.Node1, _config.Node2, _config.Node3 }.Select(i => Node(i).Address)); | ||
}, _config.Node4, _config.Node5); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The partitions here make sense [1,2,3] and [4.5]
// now it should have been unstable for more than 17 seconds | ||
|
||
// all downed | ||
AwaitCondition(() => cluster.IsTerminated, max: TimeSpan.FromSeconds(15)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense
{ | ||
AwaitAssert(() => | ||
{ | ||
Cluster.State.Members.Count.Should().Be(3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nodes that couldn't acquire the Lease
have been downed here
@@ -2639,7 +2639,7 @@ public void AssertLatestGossip() | |||
public void PublishMembershipState() | |||
{ | |||
if (_cluster.Settings.VerboseGossipReceivedLogging) | |||
_log.Debug("Cluster Node [{0}] - New gossip published [{0}]", SelfUniqueAddress, _membershipState.LatestGossip); | |||
_log.Debug("Cluster Node [{0}] - New gossip published [{1}]", SelfUniqueAddress, _membershipState.LatestGossip); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch
fix Become after acquire lease
Updated: