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
Typed persistent event adapters/wrappers #25050
Conversation
Snapshot tests will fail as i made it use leveldb, before the tests relied on the sync nature of the inmemory snapshoter |
Test FAILed. |
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.
looks like a good approach
replyProbe.expectMessage(State(1, Vector(0))) | ||
|
||
val events = queries.currentEventsByPersistenceId(pid).runWith(Sink.seq).futureValue | ||
events shouldEqual List(EventEnvelope(Sequence(1), pid, 1, Wrapper(Incremented(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.
so if one would like the unwrapped here it could be possible to use the same adapter in a map
?
*/ | ||
def withTagger(tagger: Event ⇒ Set[String]): PersistentBehavior[Command, Event, State] = | ||
copy(tagger = tagger) | ||
eventWrapper(new TaggingEventWrapper[Event](tagger)) |
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.
The we would have to support more than one EventWrapper
and then the types would go haywire?
Perhaps keep tags separate?
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.
👍
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.
Given that journals don't return the Tagged back it doesn't fit with the Wrapper anyhow
4c46cc4
to
e646458
Compare
e646458
to
2e99fc0
Compare
Test FAILed. |
Test FAILed. |
Test FAILed. |
f1f45db
to
8eb1cb7
Compare
Test FAILed. |
@patriknw this is ready for a proper review now for the scaladsl. I am now just doing the javadsl but taking a little longer than expected as our java persistent actor tests haven't been running and need some attention. |
Test PASSed. |
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.
Mostly LGTM, some minor things noted
} else { | ||
// Tags always need to be on the outside as journals match on it | ||
Tagged(setup.eventAdapter.toJournal(event), tags) | ||
} |
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.
Do the common logic outside of the if-else?
* The `callback` function is called to notify the actor that the recovery process | ||
* is finished. The default implementation logs failures at error and success writes at | ||
* debug. | ||
*/ |
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.
This scaladoc documents something else?
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.
Doh
/** | ||
* The `callback` function is called to notify the actor that the recovery process | ||
* is finished. | ||
*/ |
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.
This also seems wrong
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.
Doh
/** | ||
* Transform the event in another type before giving to the journal. Can be used to wrap events | ||
* in types Journals and Adapters understand as well as simple event migrations. | ||
*/ |
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.
How could it be used to do migration? If you already have the events they won't be adapted? Remove or clarify?
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.
Updated
Test PASSed. |
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, but needs some rebasing now.
1a3d3ff
to
429865d
Compare
Rebased and can be merged once validation passes |
This also fixes #24637 |
Test PASSed. |
Not ready for merge or code review.
Just wanted an opinion on a possible API that could replace the need for basic event adapters.
I called it wrapper for now as that is the current use case.