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

akka serialization and scalapb #27

Closed
ahjohannessen opened this issue May 27, 2015 · 18 comments
Closed

akka serialization and scalapb #27

ahjohannessen opened this issue May 27, 2015 · 18 comments

Comments

@ahjohannessen
Copy link
Contributor

Would it make sense to have com.trueaccord.scalapb.GeneratedMessage and com.trueaccord.scalapb.GeneratedEnum extend java.io.Serializable. Reason for asking is that akka serialization picks the wrong serializer due to this:

In case of ambiguity, i.e. the message implements several of the configured classes, the most specific configured class will be used, i.e. the one of which all other candidates are superclasses. If this condition cannot be met, because e.g. java.io.Serializable and MyOwnSerializable both apply and neither is a subtype of the other, a warning will be issued

I believe com.google.protobuf.GeneratedMessage implements java.io.Serializable with serialVersionUID = 1L.

thesamet added a commit that referenced this issue May 30, 2015
Fixes #27

Conflicts:
	compiler-plugin/src/main/scala/com/trueaccord/scalapb/compiler/ProtobufGenerator.scala
	scalapb-runtime/src/main/scala/com/trueaccord/scalapb/GeneratedMessageCompanion.scala
@ahjohannessen
Copy link
Contributor Author

thx 👍

@thesamet
Copy link
Contributor

This is in 0.4.15 - let me know how if this helps.

@ahjohannessen
Copy link
Contributor Author

It works great, Akka correctly picks correct serializer, thanks 👍 Also thanks for being so quick :)

@zh4ngx
Copy link

zh4ngx commented Oct 19, 2016

Which serializer should akka be picking? I'm having trouble forcing it to pick the protobuf serializer. Missing the parseFrom method - so I suspect I'm targeting the wrong one?

@thesamet
Copy link
Contributor

You need to implement your own serializer as described in http://doc.akka.io/docs/akka/snapshot/scala/serialization.html

Akka's default proto serializer is for Java generated classes.

@zh4ngx
Copy link

zh4ngx commented Oct 19, 2016

Gotcha, thanks!

On Tue, Oct 18, 2016, 6:07 PM Nadav Samet [email protected] wrote:

You need to implement your own serializer as described in
http://doc.akka.io/docs/akka/snapshot/scala/serialization.html

Akka's default proto serializer is for Java generated classes.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#27 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABRIPN3cCdaKaJ7lwgnQ90x0Qa294k3Bks5q1W04gaJpZM4Er-L7
.

@eiennohito
Copy link
Contributor

You can use my implementation for reference:
https://gist.github.com/eiennohito/5b10d6fb3fd180cfa7fb2a0634b8a3bc

I register it as

akka {
  actor {
    serializers {
      scalaProto = "ws.eiennohito.utils.serialization.ScalaProtobufSerializer"
    }

    serialization-bindings {
      "com.trueaccord.scalapb.GeneratedMessage" = scalaProto
    }
  }
}

@nattyddubbs
Copy link

nattyddubbs commented Oct 19, 2016

The Akka protobuf serializer works I have used it successfully numerous times on ScalaPB generated classes. It uses reflection to determine whether or not the class adheres to the desired contract (toByteArray parseFrom)

proto = "akka.remote.serialization.ProtobufSerializer"

@thesamet
Copy link
Contributor

@nattyddubbs Nice, I didn't know that! @ZhangBanger can you share a minimal example with the problem you are seeing?

@zh4ngx
Copy link

zh4ngx commented Oct 19, 2016

I have this in my config:

actor {
    provider = "akka.cluster.ClusterActorRefProvider"
    serializers {
      proto = "akka.remote.serialization.ProtobufSerializer"
    }

    serialization-bindings {
      "com.trueaccord.scalapb.GeneratedMessage" = proto
    }
  }

Getting this error:

[ERROR] [10/19/2016 20:40:32.731] [DistributedMessaging-akka.remote.default-remote-dispatcher-18] [akka.remote.Remoting] io.bigfast.messaging.Channel$Message.parseFrom([B)
java.lang.NoSuchMethodException: io.bigfast.messaging.Channel$Message.parseFrom([B)

(Ignore the somewhat confusing naming of Channel and Message)

@nattyddubbs @thesamet Is this because GeneratedMessage does not implement parseFrom directly and instead calls from a companion object?

@zh4ngx
Copy link

zh4ngx commented Oct 19, 2016

For a little more background, my message type looks something like:

message Channel {
    message Message {
    }
}

Another possibility is the nesting is not playing nicely? Looking into that as well.

Update - I moved Message top-level and it's no longer complaining. I don't see anything visibly different in the generated code. Any thoughts?

@thesamet
Copy link
Contributor

I think I have seen something similar before - Java reflection and nested companion objects. Let me know if this is something you'd like us to fix, by filing another issue.

@nattyddubbs
Copy link

nattyddubbs commented Oct 20, 2016

@ZhangBanger I add an individual entry for each message type that I'll be sending in between Actors in the serialization-binding entry. For example:

 serialization-bindings {
     "<yourpackagename>.Channel" = proto
     "<yourpackagename>.Message" = proto
  }

@zh4ngx
Copy link

zh4ngx commented Oct 20, 2016

@nattyddubbs are you able to deal with nested messages? I tried individual entries as well without success

@nattyddubbs
Copy link

@ZhangBanger I have not used the Akka built in Protobuf serializer for nested messages. Typically nested messages for our use cases would always remain nested and thus not need to be serialized individually.

@eiennohito
Copy link
Contributor

eiennohito commented Oct 20, 2016

@ZhangBanger my solution works for nested messages.

By the way, you can register only the common interface for akka as specified in its documentation.

If you still want to register only several your classes, you need to specify class name for them which uses $ instead of . for wrapper-wrapped separators. That is also written on akka documentation page.

@zh4ngx
Copy link

zh4ngx commented Oct 20, 2016

@eiennohito - gotcha, thanks!

@thesamet - let me take another look and see if I can figure out what happened. Would be nice to not have to write more code / add more deps.

@nattyddubbs
Copy link

@eiennohito That's really good to know. Maybe we'll look at using that approach in the future 👍

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

No branches or pull requests

5 participants