-
Notifications
You must be signed in to change notification settings - Fork 9
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
Should handle subscriptions in any state #15
Should handle subscriptions in any state #15
Conversation
[INFO][3/8/2019 7:40:11 PM][Thread 0009][[akka://test/user/$a#1722184184]] Persistence probe terminated. Recreating... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicks and small changes
sqlite { | ||
class = ""Akka.Persistence.Sqlite.Journal.SqliteJournal, Akka.Persistence.Sqlite"" | ||
auto-initialize = on | ||
connection-string = """ + "Filename=file:memdb.db;Mode=Memory;Cache=Shared" + @""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, don't need to concatenate a string here - since the connection string itself is also a string.
sqlite { | ||
class = ""Akka.Persistence.Sqlite.Snapshot.SqliteSnapshotStore, Akka.Persistence.Sqlite"" | ||
auto-initialize = on | ||
connection-string = """ + "Filename=file:memdb.db;Mode=Memory;Cache=Shared" + @""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, don't need to concatenate a string here - since the connection string itself is also a string. Same feedback as above.
@@ -25,7 +25,7 @@ public AkkaPersistenceLivenessProbeProviderSettingsTest(ITestOutputHelper helper | |||
}} }"; | |||
|
|||
|
|||
[Fact] | |||
[Fact(DisplayName = "AkkaPersistenceLivenessProbeProviderSettingsTest_Should_Load")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider giving this a human-friendly name instead - that's how it'll show up in the test run results files.
|
||
<ItemGroup> | ||
<PackageReference Include="akka.persistence.sqlite" Version="1.3.11" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the Version
to read $(AkkaVersion)
- that way we can synchronize on the Akka.NET version specific in common.props
, just like how we do for the TestKit on the line below.
@@ -113,7 +122,14 @@ protected override void PreStart() | |||
private void CreateProbe(bool firstTime) | |||
{ | |||
_probe = Context.ActorOf(Props.Create(() => new AkkaPersistenceHealthCheckProbe(Self, firstTime))); | |||
Context.System.Scheduler.ScheduleTellOnce(TimeSpan.FromSeconds(10), _probe, "hit" + _probeCounter++, Self); | |||
if(firstTime) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
||
Command<SaveSnapshotSuccess>(save => | ||
{ | ||
if (!_firstAttempt) | ||
{ | ||
DeleteMessages(LastSequenceNr - 1); | ||
DeleteSnapshots(new SnapshotSelectionCriteria(save.Metadata.SequenceNr - 1)); | ||
DeleteMessages(save.Metadata.SequenceNr - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -25,7 +25,7 @@ public AkkaPersistenceLivenessProbeProviderSettingsTest(ITestOutputHelper helper | |||
}} }"; | |||
|
|||
|
|||
[Fact(DisplayName = "AkkaPersistenceLivenessProbeProviderSettingsTest_Should_Load")] | |||
[Fact(DisplayName = "Should be able to load using hocon configuration")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be able to load PersistenceProvider
?
} | ||
}}"; | ||
[Fact(DisplayName = "AkkaPersistenceLivenessProbe_Should_Handle_Subscriptions_In_Any_State")] | ||
[Fact(DisplayName = "Should be able to accept a subsrcibe request in any state")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be able to subscribe to what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect
#13