-
Notifications
You must be signed in to change notification settings - Fork 10.6k
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
Switch C# to contextual serializer and deserializer internally #17167
Switch C# to contextual serializer and deserializer internally #17167
Conversation
|
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.
looks mostly good, a couple of minor nits
public abstract byte[] PayloadAsNewBuffer(); | ||
public virtual byte[] PayloadAsNewBuffer() | ||
{ | ||
throw new NotImplementedException(); |
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.
just wondering: why was this switched from abstract
to having a default that's just unusable? Any DeserializationContext
that doesn't implement this wouldn't work, no?
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 has been done with the idea that in the future more methods will be added (e.g. to access the payload with more efficiency) and when added, they will be added as virtual methods with a default implementation so that the backward compatibility is not broken (=classes inheriting from DeserializationContext won't break if they don't provide implementation for all methods).
So I changed the method to virtual with default implementation mostly for consistency with the methods that will be added in the future.
public abstract void Complete(byte[] payload); | ||
public virtual void Complete(byte[] payload) | ||
{ | ||
throw new NotImplementedException(); |
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.
same comment here as for the abstract -> default-throws
comment on DeserializationContext
above.
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.
Same as above
this.deserializer = EmulateSimpleDeserializer; | ||
// gRPC only uses contextual serializer/deserializer internally, so emulating the legacy | ||
// (de)serializer is not necessary. | ||
this.serializer = (msg) => { throw new NotImplementedException(); }; |
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.
can we just get rid of the serializer
and deserializer
fields entirely, since they are no longer used now?
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.
We need to keep the Serializer and Deserializer properties to maintain backward compatibility.
And the other constructor is setting the serializer and deserializer fields.
@apolcyn responded to concerns, PTAL. |
Known failures: #17170 |
contextual (de)serializers have been introduced in #16367.
Serialization performance stays the same, but the switch is a foundation for introduction of more efficient serialization is in progress in #16371