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

Event envelope updates #170

Merged
merged 37 commits into from
Jan 7, 2021
Merged

Event envelope updates #170

merged 37 commits into from
Jan 7, 2021

Conversation

eaba
Copy link
Contributor

@eaba eaba commented Jan 6, 2021

No description provided.

eaba and others added 30 commits November 12, 2020 18:30
…h missing Tests not in the latest version of 1.4.11
eaba added 2 commits January 6, 2021 11:07
* Add FluentAssertionsVersion to Common.Props and set the version to 4.14.0
* Fix failing persistence tests
@eaba
Copy link
Contributor Author

eaba commented Jan 6, 2021

Once this is merged, I will need to refork to have a clean commit history for each new branch.

Copy link
Member

@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.

Need to preserve backwards compatibility here

[BsonElement("Ordering")]
public BsonTimestamp Ordering { get; set; }

[BsonElement("Timestamp")]
public BsonTimestamp Timestamp { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

We should just use the Ordering field instead - we can't back-date the previous journal entries otherwise this will cause total chaos.

@@ -319,7 +321,7 @@ private JournalEntry ToJournalEntry(IPersistentRepresentation message)
Id = message.PersistenceId + "_" + message.SequenceNr,
//Ordering = _sequenceRepository.GetSequenceValue("journalentry"),
Ordering = new BsonTimestamp(0), // Auto-populates with timestamp
//Timestamp = new BsonTimestamp(0),
Timestamp = new BsonTimestamp(timeStamp),
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

@@ -398,12 +402,12 @@ private Persistent ToPersistenceRepresentation(JournalEntry entry, IActorRef sen
if (deserialized is Persistent p)
return p;

return new Persistent(deserialized, entry.SequenceNr, entry.PersistenceId, entry.Manifest, entry.IsDeleted, sender);
return new Persistent(deserialized, entry.SequenceNr, entry.PersistenceId, entry.Manifest, entry.IsDeleted, sender, timestamp: entry.Timestamp.Timestamp);
Copy link
Member

Choose a reason for hiding this comment

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

Use Ordering

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay

object payload = message.Payload;
message = message.WithTimestamp(timeStamp);
Copy link
Member

Choose a reason for hiding this comment

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

You can get rid of this call - it's not needed. We already have the equivalent of this with the Ordering field - just that in that instance the clock time is set by MongoDb

@@ -11,6 +11,7 @@
<ItemGroup>
<PackageReference Include="Akka.Persistence.Query" Version="$(AkkaVersion)" />
<PackageReference Include="akka.streams" Version="$(AkkaVersion)" />
<PackageReference Include="MongoDB.Driver" Version="2.11.4" />
Copy link
Member

Choose a reason for hiding this comment

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

Why? We already have a reference to MongoDb.Driver on the line below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did not know how that happened.....I will take care of it.

@@ -398,12 +403,12 @@ private Persistent ToPersistenceRepresentation(JournalEntry entry, IActorRef sen
if (deserialized is Persistent p)
return p;
Copy link
Member

Choose a reason for hiding this comment

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

need to add WithTimestamp here

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