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

[Cluster] Enable HeartbeatResponse message type #6063

Merged

Conversation

Arkatufus
Copy link
Contributor

@Arkatufus Arkatufus commented Aug 11, 2022

Fixes #6061

Changes

  • Enable HeartbeatResponse and Heartbeat protobuf wire format for cluster hearbeats.
  • Add akka.cluster.use-legacy-heartbeat-message HOCON settings, defaults to false. When set to true, the serializer will use the old legacy Heartbeat messages.
  • Add HeartbeatResponse and Heartbeat proto serializer

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.

Need to change our defaults, but otherwise LGTM

src/core/Akka.Cluster/ClusterSettings.cs Outdated Show resolved Hide resolved
src/core/Akka.Cluster/Configuration/Cluster.conf Outdated Show resolved Hide resolved
@Arkatufus
Copy link
Contributor Author

Done, changed the setting to opt-out, not opt-in

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.

I think we can skip the serializer changes and just modify

 private T AssertAndReturn<T>(T message)
        {
            var serializer = Sys.Serialization.FindSerializerFor(message);
            var serialized = serializer.ToBinary(message);
            serializer.Should().BeOfType<ClusterMessageSerializer>();
            return serializer.FromBinary<T>(serialized);
        }

to cast the returned serializer into SerializerWithStringManifest and then manually call .Manifest on that. If the serialization binding is wrong then the cast will fail.

src/core/Akka/Serialization/Serializer.cs Outdated Show resolved Hide resolved
src/core/Akka/Serialization/Serializer.cs Outdated Show resolved Hide resolved
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.

Need to harden tests

src/core/Akka.Cluster/ClusterSettings.cs Show resolved Hide resolved
@@ -63,20 +67,6 @@ public void Can_serialize_HeartbeatRsp()
AssertEqual(message);
Copy link
Member

Choose a reason for hiding this comment

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

This test only succeeds because -1,, -1 are the hard-coded values when legacy==on. Need to harden this to make sure that the correct, non--1 values are supported when legacy==off.

Copy link
Member

Choose a reason for hiding this comment

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

The AssertEqual regardless of config is what's not going to work here. Change

 var message = new ClusterHeartbeatSender.HeartbeatRsp(uniqueAddress, -1, -1);

to

 var message = new ClusterHeartbeatSender.HeartbeatRsp(uniqueAddress, 10, 3);

When legacy off, 10,3

When legacy on, -1, -1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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 - nice work @Arkatufus

@Aaronontheweb Aaronontheweb merged commit d55f67f into akkadotnet:dev Aug 16, 2022
@Aaronontheweb
Copy link
Member

TODO: need to add an issue for this letting users know about this setting as part of the Akka.NET 1.5 upgrade advisories

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.

Akka.Cluster: need to enable HeartbeatResponse message type
2 participants