Updated remoting listener extensions to support exception serialization #346
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ionSerialization. Added overload to each to allow custom IExceptionConvertors to be provided as part of the serialization/deserialization.
|
Thanks for the contribution @WhitWaldo. We are reviewing the PR. |
yashagarwal23
requested changes
Nov 22, 2022
src/Microsoft.ServiceFabric.Services.Remoting/Runtime/ServiceRemotingExtensions.cs
Show resolved
Hide resolved
src/Microsoft.ServiceFabric.Services.Remoting/Runtime/ServiceRemotingExtensions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.ServiceFabric.Services.Remoting/Runtime/ServiceRemotingExtensions.cs
Show resolved
Hide resolved
…rk 4.5, so using Enumerable.Empty instead.
…sion methods that attempts an optional ExceptionSerialization. This defaults to BinaryFormatter so as not to break existing implementations. Added another overload that allows the user to provide an aray of IExceptionConvertors. If this is provided, the user intends to use the DSC serialization. Marked the implementing extension for both Stateful and Stateless services internal so as not to confuse users with the ability to both pass an array of IExceptionConvertor while still setting the serialization technique to BinaryFormatter.
…ing the remoting listener settings from Settings.xml.
|
@yashagarwal23 Apologies for the delay in circling back here. I've applied the changes requested. Please let me know if you'd like any others made. |
|
Abandoned |
|
@olegsych I haven't abandoned this - I'm happy to make any other changes the team wants to move this forward, but since there's no community call any longer, I don't really have a good mechanism to draw any attention here. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The latest guidance on exception serialization, provides examples indicating how the instance and replica listeners need to be updated to accommodate the new serialization settings.
Upon reading these, I realized that I use a provided extension method to configure these listeners in my own code and that this serialization change was not yet realized, so I now offer this PR to resolve this.
I've added an overload to each extension method (
CreateServiceRemotingInstanceListener()andCreateServiceRemotingReplicaListener()) to allow the developer to pass in an array ofIExceptionConvertorfor any custom exceptions they may wish to accommodate. By default, the extension provides an empty array in order to avoid necessitating any developer change to use this additional functionality.In order that this might be consumed without incurring any breaking changes to remoting services, I've added an overload that matches the original signature and provides an empty array of
IExceptionConvertorto the new overload. All V1 remoting is left untouched as this change is applicable only to V2.Rider confirms that this code matches code standard requirements and the project builds locally as expected. Happy to make any changes as required by your team. Thank you for the consideration!