Skip to content

=cluster fix double-handshakes and double-cluster events #654

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
Jun 16, 2020
Merged

=cluster fix double-handshakes and double-cluster events #654

merged 18 commits into from
Jun 16, 2020

Conversation

@ktoso
Copy link
Member

@ktoso ktoso commented Jun 4, 2020 •
edited
Loading

This was a bug in the handshakes; they would wrongly continue handshaking even if we already had a handshake completed -- mistake was one missing return .same

Motivation:

  • event handshakes caused twice

Modifications:

Result:

How associations and retries are stored:

The gossip changes:

Have not been failing a long time:

Test fixup:

@ktoso
Copy link
Member Author

ktoso commented Jun 4, 2020

There will still be failures; I'm taking them on one by one.

ktoso
ktoso commented Jun 4, 2020
View reviewed changes
Tests/DistributedActorsTests/Cluster/ClusterLeaderActionsClusteredTests.swift
eventsOnFirstSub.shouldContain(.membershipChange(.init(node: second.cluster.node, fromStatus: nil, toStatus: .joining)))
eventsOnFirstSub.shouldContain(.membershipChange(.init(node: first.cluster.node, fromStatus: .joining, toStatus: .up)))
eventsOnFirstSub.shouldContain(.membershipChange(.init(node: second.cluster.node, fromStatus: .joining, toStatus: .up)))
eventsOnFirstSub.shouldContain(.leadershipChange(Cluster.LeadershipChange(oldLeader: nil, newLeader: .init(node: first.cluster.node, status: .joining))!)) // !-safe, since new/old leader known to be different
Copy link
Member Author

@ktoso ktoso Jun 4, 2020

Choose a reason for hiding this comment

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

It is not 100% specific/certain that's the events we'll get; it depends on timing when we subscribe; there could already be the .joining event in there which means the unknown -> joining will not be emitted as event since it was in the snapshot etc.

The test already automatically checks that no event is signalled twice though, so we won't get incorrect things.

ktoso
ktoso commented Jun 4, 2020
View reviewed changes
Sources/DistributedActors/Cluster/ClusterShell.swift
state.log.debug("Association already allocated for remote: \(reflecting: remoteNode), existing association: [\(existingAssociation)]")
switch existingAssociation.state {
case .associating:
()
Copy link
Member Author

@ktoso ktoso Jun 4, 2020

Choose a reason for hiding this comment

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

this was wrong (after the recent rework);

Proper retrying must be handled more properly (today we hammer on retries, but should do backoffs and giving up too) -- will be done separately (has tickets)

ktoso
ktoso commented Jun 4, 2020
View reviewed changes
yim-lee
yim-lee approved these changes Jun 4, 2020
View reviewed changes
This was referenced Jun 12, 2020
Closed
Merged
ktoso added 17 commits June 15, 2020 11:46
@ktoso
=cluster fix double-handshakes and double-cluster events; Fixes test_…
ad2498a 
…up_ensureAllSubscribersGetMovingUpEvents
@ktoso
test_up_ensureAllSubscribersGetMovingUpEvents OK
2d876ee 
test_sendingMessageToNotYetAssociatedNode_mustCauseAssociationAttempt OK
@ktoso
fix the possibility of when incoming handshake offer is being accepted
ed6cc65 
that some actors kick off another outgoing handshake still
@ktoso
remove comments/println
334e17a
@ktoso
=cluster,handshake #141 #389 #450 #604 handshakes retry (backoff) and…
7b749c2 
… give up
@ktoso
=test better logging for CRDT ActorOwned tests
9c8b709
@ktoso
=cluster,gossip Drop ConvergentGossip and use Gossip<> for membership
81ebc1d
@ktoso
=gossip implement onClusterMember peer discovery and use in Membership
8d347ae
@ktoso
=test fix test_handshake_shouldNotifyOnRejection
758692d
@ktoso
=test better logs in DowningClusteredTests
bd60cc6
@ktoso
=test silence some test logs, by using NoopLogger
43be8e7
@ktoso
=serialization avoid top-level encoded String, since JSONEncoder disl…
5f5cbbf 
…ikes it
@ktoso
=test #157 we've not seen it fail a long time, removed extra logging
6f7bdc8
@ktoso
=test adjust test that now all gossips use the same mechanism
f7708ca
@ktoso
=log fix metadata handling in LogCapture as we now rely on it for act…
3908125 
…or paths
@ktoso
=test #616 fix too aggressive assertions in test_autoUpdatedListing_u…
1d1dd36 
…pdatesAutomatically

The listing may happen a moment after; we can see a listing that is
empty at first after all
@ktoso
+cluster #670 leaving/down node MUST reject aggresively new handshakes
dcc880d
@ktoso ktoso mentioned this pull request Jun 15, 2020
Merged
yim-lee
yim-lee reviewed Jun 15, 2020
View reviewed changes
@ktoso
Copy link
Member Author

ktoso commented Jun 16, 2020

Commits have the individual issues resolved one by one, going to merge and keep hardening remaining things.

@ktoso ktoso merged commit 5de9887 into apple:master Jun 16, 2020
@ktoso ktoso deleted the fix-test_up_ensureAllSubscribersGetMovingUpEvents branch June 16, 2020 00:44
@ktoso ktoso mentioned this pull request Jun 19, 2020
Closed
@ktoso
Copy link
Member Author

ktoso commented Jul 3, 2020

The double events was:

Resolves #606

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yim-lee yim-lee yim-lee approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

FAILED: ClusterLeaderActionsClusteredTests.test_down_to_removed_ensureRemovalHappensWhenAllHaveSeenDown Better handle ensureAssociation/connect with dead node Handshake backoff, when other node is not there FAILED: CRDTActorOwnedTests.test_actorOwned_GCounter_increment_shouldResetDelta_shouldNotifyOthers Handshakes should not retry forever and backoff between attempts
Cluster: Implement retrying and giving up in handshakes FAILED: ClusterLeaderActionsClusteredTests.test_up_ensureAllSubscribersGetMovingUpEvents FAILED: test_autoUpdatedListing_updatesAutomatically test_up_ensureAllSubscribersGetMovingUpEvents: Duplicate cluster move event (confirmed using new asserts) FAILED: test_association_shouldAllowSendingToRemoteReference

2 participants

@ktoso @yim-lee