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

Remove legacy messages #1191

Closed
wants to merge 49 commits into from
Closed

Remove legacy messages #1191

wants to merge 49 commits into from

Conversation

danielmarbach
Copy link
Contributor

No description provided.

@danielmarbach danielmarbach requested a review from a team June 13, 2018 11:21
@danielmarbach
Copy link
Contributor Author

@Particular/servicecontrol-maintainers given that we are now at version 2.0 and working on essentially the next major version is it safe to remove this code?

@mikeminutillo
Copy link
Member

I think in almost all cases this should be fine BUT there's never a 100% perfectly safe way to remove these. We don;t know the version that we are upgrading from and we don't technically know what messages might be on the wire. This is part of the reason that SC shouldn't talk to itself via the transport.

@SzymonPobiega
Copy link
Member

Can we first move them to a separate legacy.cs file and make them (the handlers) log warn instead of doing actual work?

@danielmarbach
Copy link
Contributor Author

@SzymonPobiega what would be the benefit of that and how would we get that information back once that log warn occurs at a customer site?

@SzymonPobiega
Copy link
Member

@danielmarbach we would not get that information back :-( What I meant is that

  • It is OK to simplify the code to a degree that processing these messages is NOOP
  • It is not OK to simplify the code if the simplified code crashes when such a message is handled

@danielmarbach
Copy link
Contributor Author

I did some digging. We introduced this as part of 1.39 which was release 3rd April 2017. The latest 1.x release is 1.48. The major upgrade that we released was 2.0.2 and now we are essentially working on 3.0. I think it is reasonable to assume that people who installed 1.39 can catch up with the 1.x range and gradually migrate to the next major. We can even document that on the docs site. I'm for removing it but I have not enough context to make the decision. @Particular/servicecontrol-maintainers thoughts?

// cc @janovesk @WojcikMike you guys have been involved in the efforts around 1.39 maybe you can chime in as well.

@SzymonPobiega
Copy link
Member

@danielmarbach how about a behavior early in the pipline that discard messages based on the type? Then we can have a hard-coded list of message types that we no longer support. It would be a single behavior with a single list.

@danielmarbach
Copy link
Contributor Author

That comes at the cost of parsing the enclosed message type header for every message that comes into service control as well as potentially removing the assembly qualified part from the string to do a match against

            "ServiceControl.MessageFailures.InternalMessages.PerformRetry",
            "ServiceControl.MessageFailures.InternalMessages.RegisterSuccessfulRetry"

is that worth it?

@SzymonPobiega
Copy link
Member

@danielmarbach can we just used the String.StartsWith() and pass the type name followed by a comma?

@WilliamBZA
Copy link
Member

Closing this as it is probably better to be re-implemented rather than trying to rebase. Probably also worth re-discussing the points above too.

@WilliamBZA WilliamBZA closed this Jul 10, 2018
@WilliamBZA WilliamBZA deleted the legacy-messages branch July 10, 2018 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants