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

Deprecating V1 service remoting stack #395

Merged
merged 20 commits into from
Feb 25, 2025
Merged

Deprecating V1 service remoting stack #395

merged 20 commits into from
Feb 25, 2025

Conversation

plave0
Copy link
Contributor

@plave0 plave0 commented Feb 19, 2025

No description provided.

@plave0 plave0 requested a review from Copilot February 19, 2025 17:07
@plave0 plave0 self-assigned this Feb 19, 2025
Copilot
Copilot AI reviewed Feb 19, 2025
View reviewed changes

Choose a reason for hiding this comment

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

Copilot reviewed 64 out of 79 changed files in this pull request and generated no comments.

Files not reviewed (15)
  • src/Microsoft.ServiceFabric.Actors/Client/ActorProxyFactory.cs: Evaluated as low risk
  • src/Microsoft.ServiceFabric.Actors/Remoting/V1/Builder/ActorProxyGeneratorBuilder.cs: Evaluated as low risk
  • src/Microsoft.ServiceFabric.Actors/Remoting/V1/ActorEventSubscription.cs: Evaluated as low risk
  • src/Microsoft.ServiceFabric.Actors/Remoting/V1/Builder/ActorEventProxyGeneratorWith.cs: Evaluated as low risk
  • src/Microsoft.ServiceFabric.Actors/Remoting/V1/Builder/ActorEventProxyGeneratorBuilder.cs: Evaluated as low risk
  • src/Microsoft.ServiceFabric.Actors/Remoting/FabricTransport/FabricTransportActorRemotingProviderAttribute.cs: Evaluated as low risk
  • src/Microsoft.ServiceFabric.Actors/Remoting/V1/ActorMessageBodySerializer.cs: Evaluated as low risk
  • src/Microsoft.ServiceFabric.Actors/Remoting/V1/Builder/ActorProxyGeneratorWith.cs: Evaluated as low risk
  • src/Microsoft.ServiceFabric.Actors.Wcf/WcfActorRemotingProviderAttribute.cs: Evaluated as low risk
  • src/Microsoft.ServiceFabric.Actors.Wcf/Remoting/V1/Wcf/Client/WcfActorRemotingClientFactory.cs: Evaluated as low risk
  • src/Microsoft.ServiceFabric.Actors/Client/IActorProxy.cs: Evaluated as low risk
  • src/Microsoft.ServiceFabric.Actors/Remoting/V1/Builder/ActorMethodDispatcherBase.cs: Evaluated as low risk
  • src/Microsoft.ServiceFabric.Actors/Remoting/ActorRemotingProviderAttribute.cs: Evaluated as low risk
  • src/Microsoft.ServiceFabric.Actors/Remoting/V1/Client/ActorEventSubscriberManager.cs: Evaluated as low risk
  • src/Microsoft.ServiceFabric.Actors.Wcf/Remoting/V1/Wcf/Runtime/WcfActorServiceRemotingListener.cs: Evaluated as low risk
@olegsych
Copy link
Member

@plave0 A couple of thoughts that didn't fit the comments:

  • Let's be consistent about marking types and members Obsolete vs. surrounding them with pragma warning 618. VS displays obsolete types and members crossed out every time they are used, so it's a good visual indicator. It also is more compact compared to pragma disable and pragma restore directives, at least for a single item.
  • Don't forget to mark the V1 items obsolete in the remoting client and listener enums.
  • I think we should also mark the FabricTransportRemotingListenerSettings.ExceptionSerialization.BinaryFormatter enum item obsolete.
  • I think we have two attributes that need to throw InvalidOperationException one for actors and one for services.

@plave0
Deprecate GetResponseMessageBodyValue and CreateRequestMessageBody
affd536
@plave0
Deprecate remianing fields and properies
18a8bb5 
In the previous commits, some fields of types that
are part of the V1 stack where surrounded by warrning
suppresions instead of being marked as obsolete.
@plave0 plave0 force-pushed the user/pavleiri/deprecateV1RemotingStack branch from c90b404 to 18a8bb5 Compare February 20, 2025 14:52
@olegsych
Copy link
Member

@plave0 I completed another pass. I think the unresolved comments represent the outstanding issues. Could you go over them one more time? Don't forget the previous conversation comment I added above.

One thing I think we should do is replace the literal strings in the Obsolete attributes with a string constant. I suspect that we will need to figure out what to put in the deprecation message, but for now, let's at least make them all consistent. I would create a new internal static class Deprecated with a string RemotingV1 field, so that the deprecations look like [Obsolete(Deprecated.RemotingV1)]. If we can make this field static readonly, that would be my first choice, otherwise const string should be fine.

@plave0
Remove default remoting providers
4717f94 
An exception is raised if a remoting provider is not specified explicitely.
@plave0
Deprecate V1 remoting listener and client versions
b8c330e
@plave0
Create static variable for all deprecation messages
7ef4c41
@plave0
Refactor tests
306b5bb
@plave0
Refactor tests
2022c31 
ServiceRemotingProvider and ActorRemoting provider
tests to check for return values instead of exceptions.
@plave0
Implement mocks with Moq library
317bd07 
This removes boilerplait code for manual mock implementation
@plave0
Bump internal package version
5f3aee9
olegsych
olegsych requested changes Feb 25, 2025
View reviewed changes
Copy link
Member

@olegsych olegsych left a comment

Choose a reason for hiding this comment

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

I've added a lot of comments, most are cosmetic but there are several substantial ones. It should be possible to address most of them in a few hours. Could you try to get this done and complete a self-review before our scrum tomorrow?

@plave0
Formatting fixes
7caabc2
@plave0
Replace remaining deprecation messages
c928231 
Remaining refactoring form 7ef4c4
@plave0
Remove default remoting stack form ActorTypeInformation
6ef4754 
We don't need to check whether GetProvider return null, since no path return null. The expected behavior is for GetProvider to throw an exception if ActorRemotingProviderAttribute is not set explicitly, so we don't need to set a RemotingListenerVersion by default.
@plave0
Deprecate IsRemotingV1 predicate
8493aa2 
To be able to use this function in other functions that cannot be deprecated (those which implement both V1 and V2 functionality), appropriate warning disables have been places.
@plave0
Refactor tests
ad70b92 
This changes cover the case when changing the exception message is also considered an error.
@plave0
Refactor tests
da53786 
Applying some good practices
@plave0
Refactor assembly attributes for test projects
7b2309a
@plave0
Add info to RemotingV1 deprecation message
17bc8cb 
More information about the deprecation of Service Remoting V1 can be found on the link.
@plave0 plave0 marked this pull request as ready for review February 25, 2025 14:21
@plave0
Refactor tests
5633b6e 
Make naming more descriptive
@plave0
Add help link to ProviderAttribute exceptions
6ca5a3b 
This gives more information about the changes made to the new SDK version and how to handle them.
@plave0 plave0 enabled auto-merge (squash) February 25, 2025 17:25
olegsych
olegsych approved these changes Feb 25, 2025
View reviewed changes
Copy link
Member

@olegsych olegsych left a comment

Choose a reason for hiding this comment

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

:shipit:

@plave0 plave0 merged commit d8da8df into develop Feb 25, 2025
2 checks passed
@plave0 plave0 deleted the user/pavleiri/deprecateV1RemotingStack branch February 25, 2025 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@olegsych olegsych olegsych approved these changes

Assignees

@plave0 plave0

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@plave0 @olegsych