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 - Add app-version to the Member information #4577
Conversation
…#29546) * will be used in rolling update features * configured with akka.cluster.app-version
@@ -545,6 +545,7 @@ Target "Protobuf" <| fun _ -> | |||
|> append (sprintf "-I=%s" (__SOURCE_DIRECTORY__ @@ "/src/protobuf/common") ) | |||
|> append (sprintf "--csharp_out=internal_access:%s" (__SOURCE_DIRECTORY__ @@ destinationPath)) | |||
|> append "--csharp_opt=file_extension=.g.cs" | |||
|> append "--experimental_allow_proto3_optional" |
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.
needed for optional
proto members
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
/// `-` separator. The number of commits from the tag is handled as a numeric part. | ||
/// For example `1.0.0+3-73475dce26` is less than `1.0.10+10-ed316bd024` (3 < 10). | ||
/// </summary> | ||
public class AkkaVersion : IComparable<AkkaVersion>, IEquatable<AkkaVersion> |
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.
Not sure about the name. Should be called just Version
, but that would conflict everywhere with System.Version
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.
I can rename it AppVersion
after merge - that way it wouldn't be confused with the Akka.NET version, which is what I initially thought when I saw the name.
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
@@ -545,6 +545,7 @@ Target "Protobuf" <| fun _ -> | |||
|> append (sprintf "-I=%s" (__SOURCE_DIRECTORY__ @@ "/src/protobuf/common") ) | |||
|> append (sprintf "--csharp_out=internal_access:%s" (__SOURCE_DIRECTORY__ @@ destinationPath)) | |||
|> append "--csharp_opt=file_extension=.g.cs" | |||
|> append "--experimental_allow_proto3_optional" |
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
@@ -73,6 +73,7 @@ namespace Akka.Cluster | |||
public CurrentClusterState() { } | |||
public CurrentClusterState(System.Collections.Immutable.ImmutableSortedSet<Akka.Cluster.Member> members, System.Collections.Immutable.ImmutableHashSet<Akka.Cluster.Member> unreachable, System.Collections.Immutable.ImmutableHashSet<Akka.Actor.Address> seenBy, Akka.Actor.Address leader, System.Collections.Immutable.ImmutableDictionary<string, Akka.Actor.Address> roleLeaderMap) { } | |||
public System.Collections.Immutable.ImmutableHashSet<string> AllRoles { get; } | |||
public bool HasMoreThanOneAppVersion { get; } |
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.
Ran into this issue the other day with a large user cluster - looks like a good addition
@@ -45,6 +46,7 @@ public void Clustering_must_be_able_to_parse_generic_cluster_config_elements() | |||
settings.MinNrOfMembers.Should().Be(1); | |||
settings.MinNrOfMembersOfRole.Should().Equal(ImmutableDictionary<string, int>.Empty); | |||
settings.Roles.Should().BeEquivalentTo(ImmutableHashSet<string>.Empty); | |||
settings.AppVersion.Should().Be(AkkaVersion.Zero); |
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
# It has support for https://github.com/dwijnand/sbt-dynver format with `+` or | ||
# `-` separator. The number of commits from the tag is handled as a numeric part. | ||
# For example `1.0.0+3-73475dce26` is less than `1.0.10+10-ed316bd024` (3 < 10). | ||
app-version = "0.0.0" |
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 the App Version is used to specify the version of the user's software, not Akka.NET... so to take advantage of this, users will need to populate it inside their HOCONs. We'll need to add this to our cluster documentation.
/// `-` separator. The number of commits from the tag is handled as a numeric part. | ||
/// For example `1.0.0+3-73475dce26` is less than `1.0.10+10-ed316bd024` (3 < 10). | ||
/// </summary> | ||
public class AkkaVersion : IComparable<AkkaVersion>, IEquatable<AkkaVersion> |
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.
I can rename it AppVersion
after merge - that way it wouldn't be confused with the Akka.NET version, which is what I initially thought when I saw the name.
Add app-version to the Member information, (Migrated from akka/akka#29546)
will be used in rolling update features
configured with akka.cluster.app-version