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

Delete unnecessary / bad Sql.Common.Journal subscriptions #6412

Merged

Conversation

Aaronontheweb
Copy link
Member

@Aaronontheweb Aaronontheweb commented Feb 17, 2023

Changes

All of the old SubscribeTo___ methods from the Akka.Persistence.Sql.Common journal implement some bad practices, i.e. not able to detect events published in other instances of the Akka.NET processes targeting the same journal. This method deletes them and marks the old queries in Sql.Common as Obsolete. Rewrote existing queries to poll the database rather than rely on in-memory events, which is the correct way to implement these.

Also: significantly reduces allocations and overheads for writes for all Akka.Persistence.Sql.Common implementations.

Related: #5522

Checklist

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

@Aaronontheweb Aaronontheweb added this to the 1.5.0 milestone Feb 17, 2023
@Aaronontheweb Aaronontheweb marked this pull request as ready for review February 17, 2023 20:14
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.

Reviewed my changes

@@ -91,9 +91,6 @@ namespace Akka.Persistence.Sql.Common.Journal
protected virtual string ByPersistenceIdSql { get; }
protected virtual string ByTagSql { get; }
protected virtual string DeleteBatchSql { get; }
protected bool HasNewEventsSubscribers { get; }
Copy link
Member Author

Choose a reason for hiding this comment

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

No longer need these defined on the journal implementations. Could add these back to be non-breaking, but they're obsolete now regardless and not used anywhere inside the journal.

@@ -91,6 +74,10 @@ protected override bool ReceivePluginInternal(object message)
{
switch (message)
{
case SelectCurrentPersistenceIds msg:
Copy link
Member Author

Choose a reason for hiding this comment

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

Added support for proper database polling here.

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) February 17, 2023 21:24
@Aaronontheweb
Copy link
Member Author

FWIW, the Akka.Persistence.Sqlite.Tests project covers all of these changes.

@Aaronontheweb Aaronontheweb merged commit 797894c into akkadotnet:dev Feb 18, 2023
@Aaronontheweb Aaronontheweb deleted the persistence-sql-rate-limits branch February 20, 2023 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants