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

API changes roadmap for 1.0 #142

Closed
10 tasks done
Arkatufus opened this issue Nov 21, 2022 · 19 comments
Closed
10 tasks done

API changes roadmap for 1.0 #142

Arkatufus opened this issue Nov 21, 2022 · 19 comments

Comments

@Arkatufus
Copy link
Contributor

Arkatufus commented Nov 21, 2022

This issue thread is made as a discussion ground on what API needs to be changed/added into Akka.Hosting before we're freezing it for 1.0 release. We encourage users to make suggestions in this thread based on their experience in using Akka.Hosting so far.

Currently, Akka.Hosting still need these API addition/changes:

Persistence and Cluster Sharding

These API changes are needed to make persistence be compatible with sharding.

  • Add API to Akka.Cluster.Hosting to allow user to specify a specific persistence plugin for cluster sharding
  • Add API to add a persistence journal/snapshot plugin settings that can be referenced elsewhere. Modularize and optionize persistence hosting methods #146
    • Maybe we can expand AkkaPersistenceJournalBuilder for journal settings?
    • If so, we need to add AkkaPersistenceSnapshotBuilder for snapshot settings
    • Persistence plugins Hosting extensions should not use Setup classes, they should inject their HOCON configuration directly.
  • Change the current Akka.Persistence.*.Hosting implementation to not automatically inject themselves as the default persistence plugins Modularize and optionize persistence hosting methods #146
  • Change the current Akka.Persistence.*.Hosting API to let their HOCON settings be copied over or renamed to another HOCON block for use with either normal persistence or cluster sharding Modularize and optionize persistence hosting methods #146
  • Add a generalized Akka.Persistence.Hosting API to specifically set the akka.persistence.journal.plugin and akka.persistence.snapshot-store.plugin settings. Modularize and optionize persistence hosting methods #146

Remoting

  • We will not make remote deployment hosting API

Clustering

General

  • Make all options class bindable to Microsoft.Extensions.Configuration
@Aaronontheweb
Copy link
Member

Change the current Akka.Persistence.*.Hosting implementation to not automatically inject themselves as the default persistence plugins

I think for most users this is what they'd expect the plugin to do - it's only a handful of power users who want concurrent journals / snapshot stores who'd want the ability to turn this off. We should design the API to support the more common use case which is users running a single journal / snapshot store implementation.

@Aaronontheweb
Copy link
Member

Persistence plugins Hosting extensions should not use Setup classes, they should inject their HOCON configuration directly.

Ideally, but in some cases can't be avoided (i.e. using AzureCredentials for instance)

@Aaronontheweb
Copy link
Member

Expand current API with more options to configure the cluster. Current settings candidates:

I think this should be easy to do - I guess we'd need to just add these members to the ClusterOptions class?

@Aaronontheweb
Copy link
Member

Expand singleton API to enable lease

LGTM - on the Akka.Management side we probably need some integration testing with ClusterSingleton and ClusterSharding using those lease implementations, mostly to test that the core Akka.NET side of things handles them properly in those scenarios since I think they've only ever been tested with TestLease before.

@Arkatufus
Copy link
Contributor Author

Arkatufus commented Nov 22, 2022

Change the current Akka.Persistence.*.Hosting implementation to not automatically inject themselves as the default persistence plugins

I think for most users this is what they'd expect the plugin to do - it's only a handful of power users who want concurrent journals / snapshot stores who'd want the ability to turn this off. We should design the API to support the more common use case which is users running a single journal / snapshot store implementation.

Yes, this makes sense. I'm thinking of adding (tentative argument names):

  • an optional bool asDefaultPlugin argument
  • an optional string journalId argument, and
  • an optional string snapshotId argument

@to11mtm
Copy link
Member

to11mtm commented Nov 25, 2022

We should design the API to support the more common use case which is users running a single journal / snapshot store implementation.

Is this really the common case or am I a power user by default? 😅

I will say that under MSSQL, separate sharding and main journals is the default (even if I'm not separating main journals out further.)

Persistence plugins Hosting extensions should not use Setup classes, they should inject their HOCON configuration directly.

IMO setup classes for persistence would be -so nice-. One of the biggest pains I see with akka is impedence mismatches between trying to config via HOCON and config via ConfigurationBuilder. Setup classes can be painful to write to be clear but otherwise there is a definite impact on consumption by users.

Unless, of course, there is some other API-way to configure a given journalId/snapshotId via setup, at least then it's chainable...

@Aaronontheweb
Copy link
Member

I want to give #144 a look before 1.0 makes it out the door

@Arkatufus
Copy link
Contributor Author

@to11mtm
The biggest problem with setup classes right now is that only one setup class can exist for each plugin, since it relies on a dictionary lookup.
Persistence is an oddity where you have to duplicate the plugin settings to apply it to two different plugins (persistence and sharding), it even have a really weird "default" setup that got copied as a fallback for each of these copies.

The second problem with setup classes is that they're applied to the final settings classes of each plugins, not the HOCON settings itself. This creates 2 different "source of truth" for each plugins. Some plugins often depends on reading HOCON configs of other plugins directly to determine its default configuration and could not pick up the changes made by the setup classes.

@Arkatufus
Copy link
Contributor Author

Ideally, no plugin should be allowed to read other plugins HOCON configuration directly, they should access the settings of other plugins after they have been initialized (eg. by calling Cluster.Get(sys).Settings. This guarantees that both HOCON config and setup class are applied before reading these setting values.

@Aaronontheweb
Copy link
Member

@to11mtm The biggest problem with setup classes right now is that only one setup class can exist for each plugin, since it relies on a dictionary lookup.

Is that really the case for Setup - we can't have more than one registered of the same type per ActorSystem? I thought we had fixed that.

@Arkatufus
Copy link
Contributor Author

Arkatufus commented Nov 28, 2022

Yes, it is still the case:
https://github.com/akkadotnet/akka.net/blob/680e10129b8a8ec83dd7115d80e1461783dc9ad7/src/core/Akka/Actor/Setup/ActorSystemSetup.cs#L62-L65

Setup can only be retrieved by Type via the internal dictionary

@Aaronontheweb
Copy link
Member

Got it - I think I got it confused with a bug we fixed in Akka.Hosting. Wonder if we need to change that so Setups are identified via a key instead of their type?

@Arkatufus
Copy link
Contributor Author

That would be a bit messy to do. Historically, each plugin only need to use a single Setup class as their HOCON "modifier" and it is a good compromise because it offers type safety. If we were to change this to an arbitrary key, we would need to think of a way to do it safely, and how each plugins would be able to know which key it need to use.

@Aaronontheweb
Copy link
Member

Makes sense - composability could always be handled through an adapter in Akka.Hosting (i.e. merging multiple custom SerializerSetups for instance)

@wesselkranenborg
Copy link

It's also a best practice to separate the persistence storage from cluster/sharding from your own persistent actors. That's currently not possible without using hocon.

And the second wish for that is to be able to supply a DefaultAzureCredential for the persistence store (ideally for both akka.persistence.azure and mssql when running against azure database) for cluster/sharding. Right now it's only possible using a connectionstring.

@Arkatufus
Copy link
Contributor Author

@wesselkranenborg We'll work on Persistence.Azure, MSSQL will need an overhaul to support Azure

@Arkatufus
Copy link
Contributor Author

Arkatufus commented Dec 9, 2022

@to11mtm @wesselkranenborg I think I might have a solution for using multiple setups for a single persistence plugin. I'd make a wrapper Setup class that contains a Dictionary<string, JournalSetupClass>. The persistence plugin then look up its journal ID inside the multi Setup, if it finds one, it applies said Setup to its settings.

@Arkatufus
Copy link
Contributor Author

@wesselkranenborg am I right in my assumption that to use Azure AD with Azure SQL you'd have to embed the access token inside the SQL connection string?

@wesselkranenborg
Copy link

@Arkatufus it's a bit different and a lot of 'magic' is going on behind the scenes. Here you have an example on how to achieve that: https://learn.microsoft.com/en-us/azure/app-service/tutorial-connect-msi-azure-database?tabs=sqldatabase%2Csystemassigned%2Cdotnet%2Cwindowsclient.

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

No branches or pull requests

4 participants