-
Notifications
You must be signed in to change notification settings - Fork 5
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
chore: SDK implementation of SpiEventSourcedEntity #39
Conversation
patriknw
commented
Nov 26, 2024
- first stab, untested
- corresponds to EventSourcedEntitiesImpl, ReflectiveEventSourcedEntityRouter, EventSourcedEntityRouter
* first stab, untested * corresponds to EventSourcedEntitiesImpl, ReflectiveEventSourcedEntityRouter, EventSourcedEntityRouter
new CommandContextImpl(entityId, 0L, "", 0, false, MetadataImpl.of(Nil), None, tracerFactory) | ||
val context = new EventSourcedEntityContextImpl(entityId) | ||
val router = createRouter(context) | ||
router.entity._internalSetCommandContext(Optional.of(cmdContext)) |
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.
We don't need the command context when accessing emptyState
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.
Thanks, that will make this more clean and I can remove the entityId parameter again.
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.
Not yet, you need it for the EventSourcedEntityContextImpl
.
But I have been playing with the same for workflows now and I believe the id must be passed earlier and be a field on the SPI impl.
So the descriptor would be something like.
new EventSourcedEntityDescriptor(componentId, id => entitySpi(id))
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.
maybe we should call it a SpiFactory since not really a descriptor anymore.
ScalaPbAny.fromJavaProto(messageCodec.encodeJava(obj)) | ||
|
||
override def fromProto(pb: ScalaPbAny): Deserialized = | ||
messageCodec.decodeMessage(pb).asInstanceOf[Deserialized] |
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 is not correct.
decodeMessage
just return the passed PbAny
.
override def decodeMessage(value: ScalaPbAny): Any = {
value
}
Instead, we should use
def decodeMessage[T](expectedType: Class[T], bytes: akka.util.ByteString): T
And fromProto
should be
fromProto[T](expectedType: Class[T], pb: ScalaPbAny): T
// FIXME smuggling 0 arity method called from component client through here | ||
ScalaPbAny.defaultInstance.withTypeUrl(AnySupport.JsonTypeUrlPrefix).withValue(ByteString.empty()))) | ||
val metadata: Metadata = | ||
MetadataImpl.of(Nil) // FIXME MetadataImpl.of(command.metadata.map(_.entries.toVector).getOrElse(Nil)) |
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 is MetadataImpl.Empty
for this, but doesn't matter much for now.
def replyOrError(updatedState: SpiEventSourcedEntity.State): (Option[ScalaPbAny], Option[SpiEntity.Error]) = { | ||
commandEffect.secondaryEffect(updatedState) match { | ||
case ErrorReplyImpl(description, status) => | ||
val errorCode = status.map(_.value).getOrElse(Status.Code.UNKNOWN.value) |
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 is legacy.
The Effect API doesn't allow the usage of an error code anymore, but the protocol is still requiring it.
I'm sure that this will always be a None
and now falling back to Status.Code.UNKNOWN
.
We should remove this completely.
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.
I'll have to test these error scenarios, because something wasn't working without the error code
Continued in #45 |