Revisit azure queue serialization.

[jason-bragg]

To support external serializers (specifically json) we merged PRs “Modified stream types to not use fallback serializer and allow external #2330“ and “Azure queue stream provider silo to silo serialization bug fix. #2447”.

While these changes are diligent about verifying that default serialization continues to work to ensure backwards compatibility, the complexity introduced by having two versions of the types that can intermix is significant. These changes may be perfectly safe, but reasoning about them for confidence in their current state and maintaining them in the future is non-trivial.
There is also a question as to whether the backwards compatibility pattern necessitates the upgrade of services reading from the queue prior to upgrading services writing to the queue.

I am sufficiently concerned about the above to request a reconsideration of this pattern.
In some other stream providers (MemoryStreamProvider, EventHubStreamProvider) we support application layer overrides of the serialization to allow developers to adapt the stream provider to their own data formats. I am of the opinion that it is safer to introduce a similar solution to the Azure queue stream provider, with the default being the original data formatting, then introduce an alternative serializer that supports the custom serialization needs intended by the pull requests mentioned above. This would strongly protect existing consumers using the azure queue stream provider, allow new consumers to opt into using the external serializer, and enable application developers to introduce their own application specific solutions as needed.

Tagging: @Carlm-MS, @gabikliot

[gabikliot]

Of course I support NOT having 2 different types that do the same. I support consolidation. Not sure how I missed the original PR that introduced this duplication.

If you only need to support binary and json, then just add a config option to stream provider. If more formats are needed, provide a way to override.

[Carlm-MS]

The initial commit was made to address a bug in the azure queue stream provider. The bug was that the stream provider was serializing the event token and batch container using the fallback serializer. This caused any types that implement the orleans serialization methods and/or are serialized using external serializers to throw exceptions if those types also are not decorated with the SerializableAttribute. If they were decorated with the SerializableAttribute then they too would end up being serialized using the fallback serializer.

So - even though the provider was using the serialization manager for serialization, the result was that the payload was always using the fallback serializer for the entire payload -or- serialization exceptions were being thrown for every message.

The introduction of the V2 types were not in the initial commit but was added by request in order to maintain backward compatibility. If all of the tests use the V2 classes, then they will function normally. There are a lot of tests around the V2 classes deserializing the original ones. The original tokens shouldn't be used in the V2 class because it'll then use the fallback serializer.

[jason-bragg]

@Carlm-MS, the V2 classes have not yet been included with an Orleans drop, meaning it should be safe for me to refactor this in a breaking way, assuming that the original versions still work. However, I wanted to check with you to see if you were using this yet, as I don't want to cause your services further churn without discussion.

[ReubenBond]

Apologies if this isn't entirely on-topic, but it would be handy for me if DefaultMemoryMessageBodySerializer didn't rely on having access to the static SerializationManager. In trying to make SerializationManager non-static, I found the need for an IOnDeserialized hook and DI construction: https://github.com/dotnet/orleans/pull/2592/files#diff-bdb4b024b8ac0579e923da740f5c98cdR46

I'm hoping I can avoid the need to add the hook

[jason-bragg]

@ReubenBond
I agree that this is off topic, but now that we've jumped the rails... :)
I've been thinking about this a bit since reviewing your PR (which I need to get back to soon), and the solution I've come to (which may well not be a good idea), is to use ActivatorUtilities to create providers (somewhat like we do with grains), so the serialization manager can be injected. Thoughts?

[Carlm-MS]

@jason-bragg We aren't using the V2 classes yet so refactoring will not be an issue.

[jason-bragg]

@Carlm-MS, thanks
@ReubenBond, The DefaultMemoryMessageBodySerializer, is a bit more involved than I'd originally surmised. At the heart of the problem is objects that we want to send that are responsible for their own serialization/deserialization that want to use the SerializationManager.

[jason-bragg]

@ReubenBond, is non-static serialization manager slated for 1.4? If it is I'll wait for that to go in before addressing this, if not, I'll follow a similar pattern as we have for DefaultMemoryMessageBodySerializer, so we can at least apply the same solution to both cases.

[ReubenBond]

@jason-bragg I prefer we wait until after 1.4.0 to merge it, just because we plan to release 1.4.0 quite soon and I want to let it sit a while longer. (The other serialization change (#2611), which obsoletes [RegisterSerializer], ought to be in 1.4.0 to give users fair warning.)

[jason-bragg]

Agreed. Especially given the release cycle of dependent services, we should not be introducing risky changes in 1.4.
Unfortunately, that means this change will cause you some merge headaches. :/ For that I apologize.

[ReubenBond]

@jason-bragg no worries whatsoever :)

[sergeybykov]

Resolved via #2658.