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.

Sign up for GitHub

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

Jump to bottom

Change the base class of PrimitiveSerializers to SerializerWithStringManifest #4989

Merged
merged 6 commits into from
Apr 29, 2021
Merged

Change the base class of PrimitiveSerializers to SerializerWithStringManifest #4989

merged 6 commits into from
Apr 29, 2021

Conversation

Arkatufus
Copy link
Contributor

Closes #4986

Aaronontheweb
Aaronontheweb requested changes Apr 28, 2021
View reviewed changes
Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

Need to make this backwards wire compatible - we have to accept the manifests that could be produced using reflection and the old type names too, and we'll have to add unit tests to verify that.

@Arkatufus
Copy link
Contributor Author

Alright, will do

@Arkatufus
Add backward compatibility to the wire format
51b5407
@Arkatufus
Merge branch '#4986_Akka.Remote.Serialization.PrimitiveSerializers_ch…
8666551 
…ange_base_to_SerializerWithStringManifest' of github.com:Arkatufus/akka.net into #4986_Akka.Remote.Serialization.PrimitiveSerializers_change_base_to_SerializerWithStringManifest
@Arkatufus
Merge branch 'dev' into #4986_Akka.Remote.Serialization.PrimitiveSeri…
bad30f7 
…alizers_change_base_to_SerializerWithStringManifest
@Arkatufus Arkatufus requested a review from Aaronontheweb April 28, 2021 20:26
Aaronontheweb
Aaronontheweb approved these changes Apr 28, 2021
View reviewed changes
Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

LGTM

@Aaronontheweb
Copy link
Member

@Arkatufus need to do API approvals as well here

@Arkatufus
Copy link
Contributor Author

Done

@Aaronontheweb Aaronontheweb enabled auto-merge April 29, 2021 14:06
@Aaronontheweb Aaronontheweb disabled auto-merge April 29, 2021 14:06
@Aaronontheweb Aaronontheweb enabled auto-merge (squash) April 29, 2021 14:06
@Aaronontheweb Aaronontheweb merged commit 25246ac into akkadotnet:dev Apr 29, 2021
@Aaronontheweb Aaronontheweb mentioned this pull request May 10, 2021
Open
@Martin-Molinero
Copy link

Martin-Molinero commented Sep 15, 2021 •
edited
Loading

In an older akka instance I'm getting Serializer not defined for message with serializer id [6] and manifest []. Transient association error (association remains live). Cannot find manifest class [S] for serializer with id [17]. when connecting from a new akka remote 1.4.20 (or bigger), think might be related to this change, breaks connection compatibility.
Is there some potential workaround for this?
Edit: the other way around does seem to work, old version connecting to new version 🚀

@Aaronontheweb
Copy link
Member

Hi @Martin-Molinero - this looks like a new version --> old version breaking wire format change that we caused, and the reason why is because we standardized the the primitive type manifests because they varied between .NET Core and .NET Framework due to the breaking changes Microsoft introduced at the inception of .NET Core.

We're really sorry about this - we probably should have made this an opt-in feature some months after introducing this serializer so you wouldn't be hit with this issue during a live upgrade. The work around for this now is probably to either just bite the bullet and upgrade everything, or to override this serializer with the one from the v1.4.19 source code and drop it later.

@Aaronontheweb Aaronontheweb mentioned this pull request Sep 15, 2021
Closed
@Arkatufus Arkatufus mentioned this pull request Sep 15, 2021
Closed
@Aaronontheweb
Copy link
Member

@Martin-Molinero this should resolve this issue in v1.4.26 #5280

@Arkatufus Arkatufus deleted the #4986_Akka.Remote.Serialization.PrimitiveSerializers_change_base_to_SerializerWithStringManifest branch September 16, 2021 14:17
@Martin-Molinero
Copy link

@Aaronontheweb awesome ty! is there an ETA for v1.4.26?

@Aaronontheweb
Copy link
Member

We can probably ship something simple soon

@Arkatufus Arkatufus restored the #4986_Akka.Remote.Serialization.PrimitiveSerializers_change_base_to_SerializerWithStringManifest branch April 22, 2022 15:37
@Arkatufus Arkatufus deleted the #4986_Akka.Remote.Serialization.PrimitiveSerializers_change_base_to_SerializerWithStringManifest branch February 27, 2023 17:25
Arkatufus added a commit to Arkatufus/akka.net that referenced this pull request May 2, 2024
@Arkatufus
Change the base class of PrimitiveSerializers to SerializerWithString…
fdd9241 
…Manifest (akkadotnet#4989)

* Change the base class of PrimitiveSerializers to SerializerWithStringManifest

* Add backward compatibility to the wire format

* Update API Approver list

(cherry picked from commit 25246ac)
Aaronontheweb pushed a commit that referenced this pull request May 2, 2024
@Arkatufus
[1.3] Implement cross platform primitive serializer (#7182)
506f8d0 
* Change the base class of PrimitiveSerializers to SerializerWithStringManifest (#4989)

* Change the base class of PrimitiveSerializers to SerializerWithStringManifest

* Add backward compatibility to the wire format

* Update API Approver list

(cherry picked from commit 25246ac)

* cherry-picked from 6101fea

Add backward compatibility to PrimitiveSerializers (#5280)

* Add backward compatibility to PrimitiveSerializers

* Update API Approver list

* Change PrimitiveSerializer compatibility switch setting name from `use-neutral-primitives` to `use-legacy-behavior` for less ambiguity (#5290)

* Change PrimitiveSerializer compatibility switch setting name from `use-neutral-primitives` to `use-legacy-behavior` for less ambiguity

* Fix unit test

* Change default to full compatibility (on)

* Fix unit test

Co-authored-by: Aaron Stannard <aaron@petabridge.com>
(cherry picked from commit 302e3cb)

* Code cleanup

* Fix FSharp.Core package problem

* Fix CI/CD script - Change vmimage to windows-latest and ubuntu-latest

* Fix CI/CD script - Install SDK

* Bump net45 target to net452

* Fix CI/CD script

* Bump Incrementalist.Cmd to 0.9.0

* Revert "Bump net45 target to net452"

This reverts commit 2d0d76d.

* Revert "Code cleanup"

This reverts commit d72b753.

* Code cleanup

* Code cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aaronontheweb Aaronontheweb Aaronontheweb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Akka.Remote.Serialization.PrimitiveSerializers needs cross platform compatibility
3 participants
@Arkatufus @Aaronontheweb @Martin-Molinero