Skip to content

Conversation

@mhbuck
Copy link
Contributor

@mhbuck mhbuck commented Oct 4, 2023

Changes

This adds the Akka.Persistence.Redis.Hosting library to enable users to make use of the Akka Hosting approach instead of requiring hocon.

The changes made are quite heavily inspired by the MongoDb version of this change akkadotnet/Akka.Persistence.MongoDB#331

Checklist

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

Copy link
Contributor Author

@mhbuck mhbuck left a comment

Choose a reason for hiding this comment

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

Overall all these changes are consistent with other implementations.

One thing I was thinking about is there a standard persistence example that could be copied and used in this project as an example project. The current examples look quite focused on performance opposed to showing the functionality.

this AkkaConfigurationBuilder builder,
string configurationString,
PersistenceMode mode = PersistenceMode.Both,
bool autoInitialize = true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not able to determine if autoInitialize is something that is even needed in something like Redis but I have left it to be consistent.

/// </exception>
public static AkkaConfigurationBuilder WithRedisPersistence(
this AkkaConfigurationBuilder builder,
string configurationString,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Through out the solution there is an inconsistency where the setting is called configuration string but all the documentation refers to it as a connection string. The changes matches what has originally been in place. This can be changed to normalise on connection string if needed

# connection string, as described here: https://github.com/StackExchange/StackExchange.Redis/blob/master/Docs/Configuration.md#basic-configuration-strings

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see why this can be a confusion point, Redis themselves called it "Configuration" (https://stackexchange.github.io/StackExchange.Redis/Configuration.html)


### Parameters

* `configurationString` __string__
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another example of the configuration vs connection string

#nullable enable
namespace Akka.Persistence.Redis.Hosting;

public class RedisJournalOptions : JournalOptions
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The redis journal and snapshot options are separate classes but set exactly the same properties in different hocon sections.

@mhbuck
Copy link
Contributor Author

mhbuck commented Oct 4, 2023

Looking at build that was done to verify this PR it seems like tests are not getting picked up during the build process
Line 149 in this build log
https://dev.azure.com/dotnet/Akka.NET/_build/results?buildId=99156&view=logs&j=bfbacb24-142c-58dd-05df-8850b4966057&t=aaebcb03-0092-592a-3d84-6d64ae854268

It looks like this stopped somewhere during the move to central packages
image

I can see that the Mongo build.fsx has some extra entries referring to runtime etc but what I cant see is where any of this is referenced in the rest of the fsx fie
https://github.com/akkadotnet/Akka.Persistence.MongoDB/blob/8304281b5a8b583954b4c8cbfd0d93f10e5cb504/build.fsx#L98

@Arkatufus
Copy link
Contributor

PR validation CI/CD should work now

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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants