Use EventId in PersistOnEventRequests #387
Conversation
| @@ -56,15 +73,16 @@ case class DurableEvent( | |||
| localLogId: String = DurableEvent.UndefinedLogId, | |||
| localSequenceNr: Long = DurableEvent.UndefinedSequenceNr, | |||
| deliveryId: Option[String] = None, | |||
| persistOnEventSequenceNr: Option[Long] = None) { | |||
| persistOnEventSequenceNr: Option[Long] = None, | |||
| persistOnEventEventId: Option[EventId] = None) { | |||
There was a problem hiding this comment.
rename to persistOnEventId (the type anyway tells that the id is an EventId and it is somewhat easier to read and pronounce).
| * of old [[com.rbmhtechnology.eventuate.PersistOnEvent.PersistOnEventRequest]]s from | ||
| * a snapshot that do not have [[com.rbmhtechnology.eventuate.PersistOnEvent.PersistOnEventRequest.eventId]] | ||
| * set. | ||
| * @param persistOnEventEventId event id of the event that caused the emission of this event in an event handler. |
There was a problem hiding this comment.
event id of the event ... -> id of the event ... (i.e. omit first event)
| * @param processId the id of the event log the initially wrote the event | ||
| * @param sequenceNr the initial sequence number in this log. Even if in case of disaster recovery the | ||
| * event ends up at a different sequence number in this log the `sequenceNr` of the | ||
| * `EventId` remains stable. |
There was a problem hiding this comment.
Maybe we shouldn't describe the special use case in the generic context here. Mentioning the id stability should be enough IMO.
| def id: VectorTime = | ||
| vectorTimestamp | ||
| val id: EventId = | ||
| EventId(processId, vectorTimestamp.localTime(processId)) |
There was a problem hiding this comment.
Please add this as a breaking change to the release notes.
| */ | ||
| case class PersistOnEventRequest(persistOnEventSequenceNr: Long, invocations: Vector[PersistOnEventInvocation], instanceId: Int) | ||
| case class PersistOnEventRequest(sequenceNr: Long, eventId: Option[EventId], invocations: Vector[PersistOnEventInvocation], instanceId: Int) |
There was a problem hiding this comment.
For consistency reasons, either
persistOnEventSequenceNrandpersistOnEventIdoreventSequenceNrandeventIdorsequenceNrandid.
I'd prefer the first option.
There was a problem hiding this comment.
We could also use EventId instead of Option[EventId] as persistOnEventId type because in the new implementation the persistOnEventId is never optional. For missing persistOnEventIds for backwards-compatibility reason we could use null and make a special check in the handlers. This keeps the interface clean and makes backwards-compatibility an implementation detail.
OTOH this proposal is rather error prone for later extensions and PersistOnEventRequest is an internal message anyway. Should we make the request private[eventuate] and add a documentation to its persistOnEventId parameter explaining why it is optional?
WDYT?
Instead of using the in case of disaster recovery potentially unstable sequence number as id for persist on event requests use a stable EventId that is composed of the sequence number of the emitter of the event and the corresponding process id. Closes #385
volkerstampa
force-pushed
the
fix-385-dr-with-persist-on-event
branch
from
dff43d8 to
5d045c0
Compare
Instead of using the in case of disaster recovery potentially
unstable sequence number as id for persist on event requests use a
stable EventId that is composed of the sequence number of the emitter
of the event and the corresponding process id.
Closes #385