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

Akka.Cluster.Sharding AK2001 cleanup #7049

Merged

Conversation

Aaronontheweb
Copy link
Member

@Aaronontheweb Aaronontheweb commented Jan 9, 2024

Changes

Fixing all instances of AK2001 inside Akka.Cluster.Sharding and cleaning up some nullable APIs while we're at it.

Checklist

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

Working on ensuring that AK2001's rules are followed inside Akka.NET's own test suites
@Aaronontheweb Aaronontheweb added akka-cluster-sharding api-change rosyln-analyzer Issues that should be addressed via user-exposed Roslyn analyzers. labels Jan 9, 2024
@Aaronontheweb Aaronontheweb added this to the 1.5.15 milestone Jan 9, 2024
Copy link
Member Author

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

Detailed changes - going to open a separate PR for adding upgrade notices to document some of these sharding changes.

@@ -3,7 +3,7 @@ uid: AK2001
title: Akka.Analyzers Rule AK2001 - "Do not use automatically handled messages in inside `Akka.Cluster.Sharding.IMessageExtractor`s."
---

# AK2000 - Warning
# AK2001 - Warning
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed a typo in the docs here

@@ -70,7 +70,7 @@ If you want to see the [full set of changes made in Akka.NET v1.5.14, click here
| 1 | 1 | 1 | szaliszali |</PackageReleaseNotes>
</PropertyGroup>
<ItemGroup Label="Analyzers">
<PackageReference Include="Akka.Analyzers" Version="0.2.0" PrivateAssets="all" />
<PackageReference Include="Akka.Analyzers" Version="0.2.1" PrivateAssets="all" />
Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to the most recent version of Akka.Analyzers, which includes an important fix to the Roslyn Code Fix Provider for AK2001

@@ -23,15 +23,15 @@ public void RouteShardEnvelope()
{
_extractor.EntityId(_m1);
_extractor.EntityMessage(_m1);
_extractor.ShardId(_m1);
_extractor.ShardId(_m1.EntityId);
Copy link
Member Author

Choose a reason for hiding this comment

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

Taking advantage of the new ShardId methods inside the IMessageExtractor for these benchmarks, since these are now what we try to use inside the ShardRegion s themselves.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing AK2001 warnings

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing AK2001 warnings - all tests still pass.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing AK2001 warnings - all tests still pass.

@@ -111,10 +111,10 @@ public Implementation(int maxNumberOfShards, Func<Msg, string> entityIdExtractor
_messageExtractor = messageExtractor;
}

public override string EntityId(Msg message)
public override string? EntityId(Msg message)
Copy link
Member Author

Choose a reason for hiding this comment

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

We applied nullability to the IMessageExtractor interface for these methods some time ago, but never applied it to the HashCodeMessageExtractor class that implements it - first that here and you'll see it show up in the API approvals as well.

@@ -413,6 +413,7 @@ public static Config DefaultConfig()
/// This exception is thrown when the cluster member doesn't have the role specified in <paramref name="settings"/>.
/// </exception>
/// <returns>The actor ref of the <see cref="Sharding.ShardRegion"/> that is to be responsible for the shard.</returns>
[Obsolete("Use one of the overloads that accepts an IMessageExtractor instead")]
Copy link
Member Author

Choose a reason for hiding this comment

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

Marked all of the Start / StartAsync methods that accept any of the deprecated delegates as Obsolete - this also appears in the API approval files.

@@ -149,7 +149,7 @@ namespace Akka.DistributedData
///
/// In the <see cref="Update"/> message you can pass an optional request context, which the <see cref="Replicator"/>
/// does not care about, but is included in the reply messages. This is a convenient
/// way to pass contextual information (e.g. original sender) without having to use <see cref="Ask"/>
/// way to pass contextual information (e.g. original sender) without having to use Ask{T}
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed an XML-DOC warning

Copy link
Member Author

Choose a reason for hiding this comment

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

AK2001 fixes

@Aaronontheweb Aaronontheweb marked this pull request as ready for review January 9, 2024 18:48
Copy link
Contributor

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

LGTM

@Aaronontheweb Aaronontheweb enabled auto-merge (squash) January 9, 2024 18:57
@Aaronontheweb Aaronontheweb merged commit 38ad362 into akkadotnet:dev Jan 9, 2024
12 checks passed
@Aaronontheweb Aaronontheweb deleted the akka-analyzers-public-reference branch January 9, 2024 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
akka-cluster-sharding api-change rosyln-analyzer Issues that should be addressed via user-exposed Roslyn analyzers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants