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

Separate Metrics business and wire format models #7067

Merged

Conversation

Arkatufus
Copy link
Contributor

@Arkatufus Arkatufus commented Jan 18, 2024

Fixes #7065

Changes

  • Move automatically generated protobuf models to Akka.Cluster.Metrics.Serialization.Proto namespace
  • Add missing fields into the internal message classes
  • Make all protobuf generated classes internal

Checklist

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

@Arkatufus Arkatufus marked this pull request as draft January 18, 2024 20:14
Copy link
Contributor Author

@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.

Self review

Comment on lines +35 to +38
public MetricsGossip Gossip { get; }

public bool Reply { get; }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added missing fields that, before, came from the protobuf generated code

@@ -130,7 +128,7 @@ public override string Manifest(object o)

private byte[] AdaptiveLoadBalancingPoolToBinary(Metrics.AdaptiveLoadBalancingPool pool)
{
var proto = new AdaptiveLoadBalancingPool()
var proto = new Proto.AdaptiveLoadBalancingPool()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the protobuf generated codes to a new namespace, hence the change.

Comment on lines +34 to +39
public sealed class EWMA: IEquatable<EWMA>
{
public double Value { get; }

public double Alpha { get; }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added equality code and missing fields

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that in the old code, IEquatable was implemented by the protobuf generated code. We need to add this back.

@@ -40,15 +37,15 @@ public MetricsGossip(IImmutableSet<NodeMetrics> nodes)
/// </summary>
public MetricsGossip Remove(Actor.Address node)
{
return new MetricsGossip(Nodes.Where(n => !n.Address.Equals(node)).ToImmutableHashSet());
return new MetricsGossip(Nodes.Where(n => !n.Address.Equals(node)).ToImmutableHashSet(NodeMetricsComparer.Instance));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed so that the immutable hash set will use the comparer to compare hash code and equality between the NodeMetrics.

namespace Akka.Cluster.Metrics.Serialization
{
internal sealed class NodeMetricsComparer: IEqualityComparer<NodeMetrics>
Copy link
Contributor Author

@Arkatufus Arkatufus Jan 18, 2024

Choose a reason for hiding this comment

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

This is needed because NodeMetrics only uses the Address property for equality and hash code computation. This is being used internally by MetricGossip ImmutableHashSet property to determine if 2 instances of NodeMetrics are the same or not. ImmutableHashSet first uses the GetHashCode() method to retrieve items, if more than one items are retrieved, it will use the Equals() method to get the more accurate match.

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 - this is potentially a bit of a breaking change due to the way we're re-arranging public types but honestly the old coupling to Protobuf'd types was totally unacceptable and I doubt these changes will impact most users, given that the same "types" still exist with the same public members now.

@Aaronontheweb Aaronontheweb merged commit 0cb2881 into akkadotnet:dev Jan 19, 2024
12 checks passed
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.Metrics mis-uses code-genned Protobuf objects for internal business logic
2 participants