Skip to content

Bugfix #530#531

Merged
Aaronontheweb merged 5 commits into
akkadotnet:devfrom
Arkatufus:#530-BugFix
Apr 7, 2025
Merged

Bugfix #530#531
Aaronontheweb merged 5 commits into
akkadotnet:devfrom
Arkatufus:#530-BugFix

Conversation

@Arkatufus

@Arkatufus Arkatufus commented Apr 7, 2025

Copy link
Copy Markdown
Contributor

Changes

  • Add unit tests
  • Refactor all internal actors to system actors
  • Add bugfix

{
}

[Fact]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please file a GitHub issue to get this added to the TCK for Akka.Persistence.Query.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@Arkatufus Arkatufus left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

self-review

_readJournalDao,
_readJournalConfig.JournalSequenceRetrievalConfiguration)),
name: $"{_readJournalConfig.TableConfig.EventJournalTable.Name}akka-persistence-sql-sequence-actor");
name: $"{_readJournalConfig.PluginId}-{_readJournalConfig.TableConfig.EventJournalTable.Name}-akka-persistence-sql-sequence-actor");

@Arkatufus Arkatufus Apr 7, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actual fix.

Original intent:
We assume that users will only use this plugin to connect to one and only one database instance, the event journal table name was thought as unique enough an identifier for a read journal sequence actor name.

Problem:
Since Akka.Persistence.Sql can target multiple databases, it is possible that each database will use the same event journal table name and the naming convention will cause an actor name collision.

Note

Since we're using the read journal plugin id to make the actor name unique instead of the write journal plugin id, this does mean that we do not guarantee that there will only be a single read journal for a single write journal. User will need to be responsible for managing the number of read journal configurations and the read journal they instantiate to ensure system responsiveness.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM


_log = Logging.GetLogger(system, $"{_readJournalConfig.PluginId}-{nameof(SqlReadJournal)}");
_queryPermitter = system.ActorOf(
_queryPermitter = system.SystemActorOf(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Refactor all internal actors to system actors

default);

_journalSequenceActor = system.ActorOf(
_journalSequenceActor = system.SystemActorOf(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Refactor all internal actors to system actors

Comment on lines +26 to +35
/// <summary>
/// Note that this is safe to do because the only place this is being called is
/// inside the `PersistenceQuery.ReadJournalFor{T}()` public method inside
/// Akka.Persistence.Query and the result of that method call is then cached
/// and reused for the duration of the ActorSystem lifetime.
/// </summary>
/// <returns>
/// A new instance of IReadJournal specific to this persistence plugin.
/// </returns>
[InternalApi]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Add explanation why this is safe to do

@Aaronontheweb Aaronontheweb left a comment

Copy link
Copy Markdown
Member

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) April 7, 2025 15:29
@Aaronontheweb Aaronontheweb merged commit ed1cef3 into akkadotnet:dev Apr 7, 2025
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