I was reviewing your older Bond PR just yesterday and was thinking about exactly the same problem!

In some cases Orleans won't generate its own serializer and external serializer will work (http://dotnet.github.io/orleans/Advanced-Concepts/Serialization -> Auto Generated Serializers). But in a lot of cases it will, and then the code, the way it is written now, will use the Orleans serializer instead of the default one. So we definitely need to solve it.

Your proposed solution of a custom attribute is OK, and will work for Bond case I guess. But it will not work for other cases. In other external serializer frameworks, the C# class is auto generated from some language independent spec. Therefore, you cannot annotate it with your custom attribute (you don't want to change/mess with the auto generated code). I have specific serializers examples in mind.

So to be general and solve this problem for all serializers we need a different way.
I can think about a number alternatives:

1. Specify in the config a list of namespaces for which "do not create Orleans serializer"
2. Change the order of discovery of serializers in the SerializatiionManager so the external one, if defined and exists, takes preference (this is basically implicitly enforcing suggestion 3).
3. Do not change the order, but just make sure the external serializer, if exist, overwrites the default Orleans's one.

I think I like the last suggestion the most: we explicitly define and document the order in which serializers are used: external first (if multiple externals exist take the first one in the list), then Orleans's one if exist, then fallback one (binary or Json). This is per type of course.

I don't think the problem is that the codegen is creating serializers, as much as that these serializers are used by default. Instead of starting with a set of pre existing serializers (partially populated with codegen created serializers), I suggest we start with an empty list, and look for the serializer for a type in the following order:

1. Check externally provided serializer, if provided
2. Check pre-existing (and code generated) serializers
3. Check default serializer.
   Once a serializer is found, it is registered and used for that type from then on.

This change should fix the problem. I removed the attribute and forced external serializers to be considered prior to falling back to the default serializer.

@jason-bragg , I think you echoed exactly what I said. :-)

We should also update the docs explaining the option of external serializers in general, and the new ordering rules as well.

I had another idea,how to improve this code even more.
We should generalize the concept of ISerializer to all our serializers. Basically, refactor the built in Orleans serializer to be yet another ISerializer, just like any other external serializer and like done already by @dVakulen in #1047.
That way we will have 3+N serializers: Built-In Orleans, .NET binary, JSON and N external.
We will treat them all the same and configuration will determine the order in which they are checked to see if certain type is supported by this serializer. If 1st serializer in the list does not support it, we ask the 2nd one, etc. That way we can change the order in any arbitrary way, while the rest of the code in SerializationManager will not have to change.
That generalization will simplify the code in SerializationManager where now we explicitly determine the order in each and every of the Copy/Ser/deser method. It also allow us in the future to add common features to all serializers in a uniform way.

@gabikliot I believe the attribute-based serializer will also have to be in that list, so the default order will be:

1. attribute-based serializer
2. build-in orleans serializer

What is "attribute-based serializer"?
Do you mean the external serializer? Yes, it (or if multiple) will be first one in the list. Although this - the order of serializers in the list - can be changed through configuration, with what we suggested here as the default.

types that have a CopierMethodAttribute, SerializerMethodAttribute, and DeserializerMethodAttribute defined

Yes, of course, I forgot about those.

@gabikliot @Carlm-MS I agree with Gabi's suggestion to generalize this in the future, and have the current order as the default configuration. Still I think that that change could have a little lower priority once we get this PR committed (as there is no reason IMO to not fix this right away and generalize the approach later).
Do you guys agree?

+1

I agree the generalization can wait after this and @dVakulen in #1047 are in.

BTW @Carlm-MS, do you happen to have any tests that verify this? I'm running all of our suite, but it still might be nice to have something that asserts the new order, especially since we intend to generalize this in the future (but it should still work in the same way by default).

@Carlm-MS I'm about to pull the changes locally to rebase and squash (as the 1st commit doesn't add a lot of value after we changed direction). You will still be listed as the author of the 1 commit. Are you cool with that or do you prefer to rebase and squash on your side?

@jdom I'm fine with it.

• added some unit tests for serializer ordering.
• fixed a minor pre-existing bug where serializers rather than deserializers were checked for logging purposes.
• discovered/fixed an issue with arrays and types handled by external serializers
• squashed commits

• discovered/fixed an issue with arrays and types handled by external serializers

Not sure I see which line of code this comment refers to, would you mind pointing me to it?

@jdom SerializationManager::HasOrleansSerialization

just added the check to see if the the external serializers handle the type.

@jdom, you should join the DotNet organization on GH (https://github.com/dotnet), so others can assign issues to you. One can assign issues only on his/project org members.

@gabikliot We are already figuring it out with .NET Foundation folks.

This is ready to be merged, right?

I think @jdom was still reviewing. OK form my side.

Yes, it’s ready. I didn’t get permissions to commit yet, that's why I didn't do it. But feel free to merge it yourself.

Thank you, @Carlm-MS!