-
Notifications
You must be signed in to change notification settings - Fork 35
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
fix: Do not special handle protobuf payloads for replication #1212
fix: Do not special handle protobuf payloads for replication #1212
Conversation
f122c68
to
0e95c38
Compare
We have verified that this solves our internal problem with protobuf any:s. There is a compatibility issue with the change though, if a user is using protobuf generated message classes to represent events in their replicated entities, we will encode them as I wonder if the risk for such a specific use is small enough that we can live with that potential wire incompatibility during a rolling upgrade? |
Thinking some more about it: if a user used protobuf generated classes for replicated events, it wouldn't work because of the bug/shortcoming we are fixing here, it would never deserialize since the receiving side did not have the proto type to descriptor mapping. What did work before is the pass-through Java and Scala protobuf Any class, it will now be forwarded to the deserializer configured, so the wire incompatibility could happen if the found Akka serializer infra does not handle Any. We could perhaps sort that by passing it through to the user as is (but even more unlikely that anyone is actually doing this). |
ScalaPbAny(akkaSerializationTypeUrl(id, manifest), UnsafeByteOperations.unsafeWrap(bytes)) | ||
} | ||
|
||
override def deserialize(event: ScalaPbAny): Any = { |
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 haven't looked in detail at this PR, but could we keep deserialization side as it was? If we serialize with AkkaSerializationTypeUrlPrefix that would still be deserialized correctly?
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 think that if we can have no special casing that is better and less trickery to try to understand, all messages are fed to the configured akka serializer. The any-passthrough is such a weird special case that it is very unlikely that anybody else than us ourselves is doing that.
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.
ok
0a39d24
to
30f4a74
Compare
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.
looking good
akka.actor.provider = cluster | ||
akka.actor { | ||
serializers { | ||
my-replication-serializer = "akka.projection.grpc.replication.ReplicationProtoEventIntegrationSpec$$ProtobufSerializer" |
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.
wouldn't it be more common to use the built in proto serializer in Akka (enabled by default), or what is the reason for defining your own?
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 didn't think of it, changed to that one now.
// FIXME issue #702 This probably means that one GrpcReadJournal instance is created for each Projection instance, | ||
// and therefore one grpc client for each. Is that fine or should the client be shared for same clientSettings? | ||
|
||
val wireSerialization = new DelegateToAkkaSerialization(system) |
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 apply is the same as the other apply above, aside from the serialization. Where is the other used? Do we need both? Is it even so that we can look at replicationSettings.isDefined
to select serializer?
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.
Simplified to a single internal apply factory now.
* INTERNAL API | ||
*/ | ||
@InternalApi | ||
private[akka] trait AkkaProjectionGrpcSerialization { |
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 we have to include AkkaProjection in the name?
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.
Dropped
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
We had special logic in place for more convenient handling of protobuf payloads when using Akka Projection directly, however when using that together with replication and protobuf any payloads it causes problems, this disables all special handling of payloads when using replication and always delegates to Akka serialization.