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

Attempt to make the serialization manager non static #2100

Closed
Drawaes opened this issue Aug 27, 2016 · 12 comments
Closed

Attempt to make the serialization manager non static #2100

Drawaes opened this issue Aug 27, 2016 · 12 comments
Assignees
Milestone

Comments

@Drawaes
Copy link
Contributor

Drawaes commented Aug 27, 2016

I propose that I have a go at this, it would be an opportunity to clean it up at the same time while hopefully helping with testing (being able to isolate the serialization during tests), it might help the DI story or even testing swapping wire in place if people feel the need for that.

I have created a branch https://github.com/Drawaes/orleans/tree/SerializationRefactor that I will start work on.

Initially I am mostly going to just try to remove the static, and make a singleton, but if there are any ideas of what would like to be seen at the same time, or if it is something that is not wanted please to say.

@ReubenBond
Copy link
Member

This will require a change to codegen, since generated seralizers use the static members. I wonder if it would be more fruitful to port to Wire instead of this and replace our serializer entirely.

@dVakulen
Copy link
Contributor

I've made rough implementation of Wire port, and it doesn't yet supports IDictionary - so I'd suggest to wait until it stabilizes (probably until Akka.Net reaches 1.5 version -https://github.com/akkadotnet/akka.net/releases/tag/v1.0.7).

@ReubenBond
Copy link
Member

Likely we will need to register default surrogates for each of those generic collections like we do in RegisterBuiltInSerializers - but point taken.

@Drawaes
Copy link
Contributor Author

Drawaes commented Aug 27, 2016

I think if we can make it non static to start with, it makes it easier to plugin and isolate. The idea being it could become an Iserializationmanager and the code gen could take an instance before starting which is what @jdom sugessted to me. Its really the first step.

@Drawaes
Copy link
Contributor Author

Drawaes commented Aug 28, 2016

It will have to be a think about how all the parts (message handling, code gen etc) take a reference to the instance based serialization manager. And to see if we can rationalize the various calls to get it to what could be a sensible interface in the future to allow the plugging of a totally different serialization from the core. I am open to suggestions?

@dVakulen
Copy link
Contributor

dVakulen commented Aug 29, 2016

Are you open to suggestions, @Drawaes? If you are - here's mine: serialization manager already supports swapping of the serializers, DI can be easily added into it as well. You can think of it as of factory method container. So making it non-static will require major unnecessary effort. Instead I'd suggest to make inner refactoring of it in order to make all serializers inherited from one interface, e.g. IExternalSerializer.

@Drawaes
Copy link
Contributor Author

Drawaes commented Aug 29, 2016

Absolutely open to suggestions. So are you saying pull out the internal "serialization" bits and move them into a IExternalSerializer and then make that the first class serialization source?

@dVakulen
Copy link
Contributor

dVakulen commented Sep 4, 2016

Yes, kind of.

@sergeybykov
Copy link
Contributor

@ReubenBond, do you think this makes sense and relatively easy to do as a follow-up to #2345?

@sergeybykov sergeybykov added this to the Triage milestone Nov 5, 2016
@ReubenBond
Copy link
Member

Yes, this is likely the next PR after that one.

On Nov 4, 2016 6:23 PM, "Sergey Bykov" notifications@github.com wrote:

@ReubenBond https://github.com/ReubenBond, do you think this makes
sense and relatively easy to do as a follow-up to #2345
#2345?


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

@sergeybykov sergeybykov modified the milestones: 1.4.0, Triage Nov 5, 2016
@sergeybykov sergeybykov modified the milestones: 1.5.0, 1.4.0 Jan 14, 2017
@sergeybykov
Copy link
Contributor

#2592 is meant to address this.

@ReubenBond
Copy link
Member

Fixed by #2592 :)

@dotnet dotnet locked as resolved and limited conversation to collaborators Sep 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants