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

Update Protobuf Serializer #8334

Merged
merged 6 commits into from
Mar 7, 2023
Merged

Update Protobuf Serializer #8334

merged 6 commits into from
Mar 7, 2023

Conversation

larsfjerm
Copy link
Contributor

@larsfjerm larsfjerm commented Feb 27, 2023 •
edited by ghost
Loading

Updated the Protobuf serializer to v7 (Protobuf-net not included), based it mostly on the STJ serializer.

Microsoft Reviewers: Open in CodeFlow

@larsfjerm
Copy link
Contributor Author

@dotnet-policy-service agree

@ReubenBond
Copy link
Member

Thank you, @larsfjerm! Could you add tests similar to the JsonCodec ones?

@ReubenBond
Copy link
Member

@abe545, any thoughts on this PR? I will give it a final review and merge it soon.

@larsfjerm
Copy link
Contributor Author

larsfjerm commented Mar 7, 2023 •
edited
Loading

I converted to ISpecializableCopier in https://github.com/larsfjerm/orleans/tree/protobuf-specialized-copier

This test fails with that copier since they are not Same instance, but Equal:
https://github.com/larsfjerm/orleans/blob/16a32b18f428aac28c3bd15e79d332614b5e591e/src/Orleans.Serialization.TestKit/CopierTester.cs#L95

I also had a try with ISpecializableCodec, but more tests are failing here, so I would need to investigate - maybe for a later PR?

@abe545
Copy link
Contributor

abe545 commented Mar 7, 2023

@abe545, any thoughts on this PR? I will give it a final review and merge it soon.

I'll pull it down today and test it with our code base and report back shortly.

@abe545
Copy link
Contributor

abe545 commented Mar 7, 2023

@abe545, any thoughts on this PR? I will give it a final review and merge it soon.

I'll pull it down today and test it with our code base and report back shortly.

@ReubenBond this PR looks great from my perspective. All my test cases passed with this implementation.

@ReubenBond
Copy link
Member

maybe for a later PR?

Sounds good to me, @larsfjerm!

Thank you for taking a look, @abe545!

@ReubenBond ReubenBond merged commit dd4321f into dotnet:main Mar 7, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@abe545 abe545 abe545 left review comments

@ReubenBond ReubenBond ReubenBond left review comments

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

Successfully merging this pull request may close these issues.

Protobuf support in version 7.x
3 participants
@larsfjerm @ReubenBond @abe545