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.Persistence.Query: include more descriptive ToString() values for all Offset types #6927

Closed
Aaronontheweb opened this issue Sep 20, 2023 · 1 comment · Fixed by #6978

Comments

@Aaronontheweb
Copy link
Member

Is your feature request related to a problem? Please describe.

I just had to write the following class in my own application that is making use of Akka.NET for EventSourcing and CQRS using Akka.Persistence.Query:

public static long AsLong(this Offset offset)
{
    switch (offset)
    {
        case Sequence seq:
            return seq.Value;
        case NoOffset _:
            return 0;
        default:
            throw new NotSupportedException($"Offset type [{offset.GetType()}] is not supported.");
    }
}

This is because the default ToString() values for Offset aren't helpful - from one of my test logs:

PublisherCreated { PublisherId = ad07e579-69f5-4738-8b20-e6a0dd3a058d, CurrentStatus = Active, Timestamp = 9/20/2023 6:39:57 PM +00:00 }] - projection state updated to [Akka.Persistence.Query.Sequence]

Describe the solution you'd like

Each Offset implementation should clearly log its value so users can plop that Offset directly into log statements without needing to filter through the underlying types. Only users with detailed knowledge about the internals of Akka.Persistence would know how to do this today.

Describe alternatives you've considered

It might be worth just exposing a long value type on each Offset anyway to help make it easier to serialize / persist too, but that gets into icky territory if someone wants to start using stuff like TimeUuids for ordering records inside the Akka.Persistence event journal - hence why the Offset abstraction exists in the first place.

Cleaning up the ToString() method for logging purposes makes the most sense.

@Aaronontheweb Aaronontheweb added this to the 1.5.14 milestone Sep 20, 2023
YariSPB added a commit to YariSPB/akka.net that referenced this issue Oct 21, 2023
Each Offset implementationclearly logs its value via ToString() so users can include that Offset directly into log statements without needing to filter through the underlying types.
@YariSPB
Copy link
Contributor

YariSPB commented Oct 21, 2023

Hi,
I am interesting in solving this issue, and have prepared a potential solution at a new branch:
https://github.com/YariSPB/akka.net/tree/6927-Akka.Persistence.Query

Does my proposal seem sound, and if so, which .txt file should I change to add new public API ToString()?

I couldn't find this file:
src/core/Akka.API.Tests/CoreAPISpec.ApproveCore.approved.txt

Maybe I should append to:
\src\core\Akka.API.Tests\verify\CoreAPISpec.ApprovePersistenceQuery.Core.verified.txt
instead?

Aaronontheweb pushed a commit that referenced this issue Oct 31, 2023
…r all Offset types #6927 (#6978)

* #6927
Each Offset implementationclearly logs its value via ToString() so users can include that Offset directly into log statements without needing to filter through the underlying types.

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