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

chore: Rewrite serialization #61

Merged
merged 11 commits into from
Dec 6, 2024
Merged

chore: Rewrite serialization #61

merged 11 commits into from
Dec 6, 2024

Conversation

patriknw
Copy link
Member

@patriknw patriknw commented Dec 5, 2024

No description provided.

@@ -200,6 +203,97 @@ private[akka] object AnySupport {
}

def extractBytes(bytes: ByteString): ByteString = bytesToPrimitive(BytesPrimitive, bytes)

def toScalaPbAny(bytesPayload: BytesPayload): ScalaPbAny = {
Copy link
Member Author

Choose a reason for hiding this comment

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

Need this for the commands that come from the fixed component endpoint (grpc)

Copy link
Member

Choose a reason for hiding this comment

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

Strange, if that's the case, we are receiving BytesPayload from outside and making a PbAny.

I think the flow is as follow:

  • static endpoint receives json
  • http grpc transcoding makes of PbAny in the runtime
  • payload is added to EntityCommand
  • inside ESE in runtime, payload is converted to BytesPayload and SPI is called

Then, we will always receive BytesPayload, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we receive BytesPayload, but with proto contentType so it's not handled by JsonSerializer
these BytesPayload<->PbAny conversions are temporary

* Parse the bytes to object of type corresponding to the type name in the `bytesPayload.contentType`. Requires that
* the types are known by first `registerTypeHints` or calling `contentTypeFor` or `toBytes`.
*/
def fromBytes(bytesPayload: BytesPayload): AnyRef = {
Copy link
Member Author

Choose a reason for hiding this comment

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

This was the case where the StrictJsonMessageCodec was used. The event types are registered up front and can then be deserialized without the expectedType: Class[T]


}

class JsonSerializer {
Copy link
Member Author

Choose a reason for hiding this comment

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

We will probably support other formats, such as proto. I haven't designed for that yet. Wanted to keep it as simple as possible first.

private def _setCurrentState(state: S): Unit = {
def handleEvent(event: E): S = {
event match {
// FIXME can it be proto?
Copy link
Member

Choose a reason for hiding this comment

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

I don't think so. Now this is always the non-serialized event java type.

@@ -200,6 +203,97 @@ private[akka] object AnySupport {
}

def extractBytes(bytes: ByteString): ByteString = bytesToPrimitive(BytesPrimitive, bytes)

def toScalaPbAny(bytesPayload: BytesPayload): ScalaPbAny = {
Copy link
Member

Choose a reason for hiding this comment

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

Strange, if that's the case, we are receiving BytesPayload from outside and making a PbAny.

I think the flow is as follow:

  • static endpoint receives json
  • http grpc transcoding makes of PbAny in the runtime
  • payload is added to EntityCommand
  • inside ESE in runtime, payload is converted to BytesPayload and SPI is called

Then, we will always receive BytesPayload, no?

value = encodeByteArray(ByteStringUtils.toProtoByteStringUnsafe(bytesPayload.bytes)))
}

private def encodeByteArray(bytes: ByteString): ByteString = {
Copy link
Member

Choose a reason for hiding this comment

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

This is a little confusing. I know this code comes from the runtime. Should we assume that is temporary? Because I was expecting the SDK to only know what a BytesPayload is.

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, so suggestions/questions about this method and it's usage just above.

Better if toProtoByteStringUnsafe is responsible for adding the proto tag.
Or this encodeByteArray should be called addProtoTag or something like that.

@patriknw
Copy link
Member Author

patriknw commented Dec 6, 2024

Failing tests from local run:

[error] Failed tests:
[error] 	akkajavasdk.SdkIntegrationTest
[error] 	akkajavasdk.EventingTestkitDestinationTest
[error] 	akkajavasdk.EventingTestkitTest
[error] 	akkajavasdk.KeyValueEntityTest
[error] 	akkajavasdk.WorkflowTest

Well merge anyway so that we can work in parallel on the different parts.

@patriknw patriknw marked this pull request as ready for review December 6, 2024 10:01
@patriknw patriknw merged commit 3b3e527 into java-spi Dec 6, 2024
7 of 8 checks passed
@patriknw patriknw deleted the wip-bytes2-patriknw branch December 6, 2024 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants