Pavel. > Yes, but they are registered separately for .NET and Java, please see how > MarshallerContextImpl stores typeId->typeName mappings.
This will not change. We still will have separate type registration. > With the proposal we put all class names in the same pile. Implementation of type registration will not be changed. What I propose is to simplify user life in the exact places we can do it for free. > Now imagine that the user has a cluster with C# and Java clients. > C# client uses class "foo.bar.Aa" for Compute, > Java client uses class "foo.bar.BB" for Services - independent use cases, > all is well. I propose to implicitly register classes only in the case they are sent to the Java side. So, for all classes that is used only in .Net nothing will change. > 13 янв. 2021 г., в 15:14, Pavel Tupitsyn <ptupit...@apache.org> написал(а): > >> Classes MUST be registered to be used in compute or cache. > > Yes, but they are registered separately for .NET and Java, > please see how MarshallerContextImpl stores typeId->typeName mappings. > > With the proposal we put all class names in the same pile. > > Now imagine that the user has a cluster with C# and Java clients. > C# client uses class "foo.bar.Aa" for Compute, > Java client uses class "foo.bar.BB" for Services - independent use cases, > all is well. > > As soon as we put all class names in the same map, the app breaks, > because "foo.bar.Aa" and "foo.bar.BB" have the same hash code, > and DuplicateTypeIdException is thrown by Ignite. > > On Wed, Jan 13, 2021 at 1:54 PM Nikolay Izhikov <nizhi...@apache.org> wrote: > >> Hello, Pavel. >> >>> Imagine that some Ignite user has lots of .NET and Java classes being >> stored in cache, used in compute, >> >> Classes MUST be registered to be used in compute or cache. >> So, in your example, user registered classes by hand, already. >> >>> Let's add a flag like BinaryConfiguration.AssumeSameJavaTypeNames, >> >> We already have this «flag». >> It a default INameMapper implementation that assumes we use the same class >> names both in Java and .Net. >> >>> This is not up for discussion, we do not break compatibility like this. >> >> Sorry, I don’t see compatibility issues in the proposal. >> Can you, please, provide an example? >> >>> 13 янв. 2021 г., в 13:46, Pavel Tupitsyn <ptupit...@apache.org> >> написал(а): >>> >>> Nikolay, >>> >>> I agree with your proposal, with one addition: >>> this behavior must be disabled by default for compatibility reasons. >>> >>> Imagine that some Ignite user has lots of .NET and Java classes being >>> stored in cache, >>> used in compute, etc. Now with the proposal implemented, all .NET classes >>> are registered >>> as Java classes too, which has a potential for hash code collisions, >>> resulting in DuplicateTypeIdException. >>> >>> So the user can't upgrade to 2.11, their app does not work anymore, which >>> is not acceptable. >>> This is not up for discussion, we do not break compatibility like this. >>> >>> Let's add a flag like BinaryConfiguration.AssumeSameJavaTypeNames, >>> which enables the behavior globally and both ways: >>> all type names become shared across all platforms in >> MarshallerContextImpl. >>> >>> >>> On Wed, Jan 13, 2021 at 11:21 AM Nikolay Izhikov <nizhi...@apache.org> >>> wrote: >>> >>>> Hello, Pavel >>>> >>>> My proposal is the following: >>>> >>>> *When Ignite configured with FQN NameMapper.* >>>> And user invokes >>>> >>>> 1. Service method from .Net to Java. >>>> 2. Compute methods `ExecuteJavaTask`, `ExecuteJavaTaskAsync` >>>> 3. Cache operations. >>>> >>>> Ignite should implicitly register types both for .Net and Java platform. >>>> >>>> ===== >>>> My intentions is to simplify usage of the Ignite .Net platform by >>>> eliminating necessity of BinaryConfiguration in .Net. >>>> >>>> Approach when user should modify Ignite configuration on both sides Java >>>> and .Net every time he use new class as a service parameter looks ugly, >> for >>>> me. >>>> >>>> You can see an example of my idea in services test [1] >>>> In current release, user forced to enlist each type in configuration. >>>> Now, you just deploy Java service and use it. >>>> >>>> Please, Note: >>>> >>>> 1. FQN NameMapper is a default value. >>>> 2. FQN NameMapper requires from the user to have exactly same names for >>>> custom binary types in Java and .Net. >>>> 3. Improvals for Services in master, already. >>>> >>>> [1] >>>> >> https://github.com/apache/ignite/blob/master/modules/platforms/dotnet/Apache.Ignite.Core.Tests/Services/ServiceTypeAutoResolveTest.cs#L146 >>>> >>>>> 13 янв. 2021 г., в 10:13, Pavel Tupitsyn <ptupit...@apache.org> >>>> написал(а): >>>>> >>>>> Nikolay, >>>>> >>>>> As I understand, you insist on changing the default behavior, is this >>>>> correct? >>>>> >>>>> >>>>> On Tue, Jan 12, 2021 at 9:51 PM Nikolay Izhikov <nizhi...@apache.org> >>>> wrote: >>>>> >>>>>> Hello, Igor, Pavel. >>>>>> >>>>>> Thanks for the feedback >>>>>> >>>>>>> Agree with Pavel, this should be disabled by default. >>>>>> >>>>>> Right now, Ignite, by default, forcing users to enlist every single >>>> value >>>>>> object they have in project in the config. >>>>>> Do you think it’s a right way to go? >>>>>> >>>>>>> To me it looks pretty dangerous as users do not explicitly control >>>>>> what’s going to be registered and it could lead to hard-to-debug >>>> mistakes >>>>>> when wrong classes get registered or with wrong names. >>>>>> >>>>>> Approach with transparent type registration implemented in every RPC >>>>>> framework I know. >>>>>> I think user expect it from the modern software. >>>>>> Can you, please, highlight dangerous part of the approach? >>>>>> >>>>>>> Wouldn't >>>>>>> it be more reasonable to only register those which are used with Java >>>>>>> services? >>>>>> >>>>>> Correct. Will do it. >>>>>> >>>>>>> Registering every .NET type as a Java type can lead to typeId >>>> collisions >>>>>> and break existing user code, >>>>>> >>>>>> For Service and Compute API binary type registration both in Java and >>>> .Net >>>>>> will happen anyway. >>>>>> It’s required just to get things work. >>>>>> >>>>>>> 12 янв. 2021 г., в 20:31, Igor Sapego <isap...@apache.org> >> написал(а): >>>>>>> >>>>>>> Agree with Pavel, this should be disabled by default. >>>>>>> >>>>>>> To me it looks pretty dangerous as users do not explicitly control >>>> what's >>>>>>> going to be >>>>>>> registered and it could lead to hard-to-debug mistakes when wrong >>>> classes >>>>>>> get >>>>>>> registered or with wrong names. Also it can be hard to use with >> classes >>>>>>> that should >>>>>>> not be registered at all, which is often the case because not >> everyone >>>>>> use >>>>>>> .NET client >>>>>>> with Java services. >>>>>>> >>>>>>> To me it looks familiar to another of our features - peer class >>>> loading, >>>>>>> which is disabled >>>>>>> by default for similar reasons. >>>>>>> >>>>>>> Also, why register types which are used as method arguments for any >>>>>>> service? Wouldn't >>>>>>> it be more reasonable to only register those which are used with Java >>>>>>> services? >>>>>>> >>>>>>> Best Regards, >>>>>>> Igor >>>>>>> >>>>>>> >>>>>>> On Tue, Jan 12, 2021 at 7:35 PM Pavel Tupitsyn <ptupit...@apache.org >>> >>>>>> wrote: >>>>>>> >>>>>>>> Nikolay, >>>>>>>> >>>>>>>> I think this behavior should be opt-in - let's add a flag to >>>>>>>> BinaryConfiguration. >>>>>>>> Registering every .NET type as a Java type can lead to typeId >>>> collisions >>>>>>>> and break existing user code, >>>>>>>> so we can't enable this by default. >>>>>>>> >>>>>>>> >>>>>>>> Ignite stores typeId -> className mappings separately for Java and >>>> .NET >>>>>>>> [1]. >>>>>>>> Separate maps were introduced because C# and Java have different >>>> naming >>>>>>>> conventions, >>>>>>>> typically you have org.foo.bar.Person in Java and Foo.Bar.Person in >>>>>> .NET, >>>>>>>> so we allow the users to map two different type names to the same >>>> typeId >>>>>>>> with a custom name or id mapper. >>>>>>>> >>>>>>>> This proposal says we should always put Foo.Bar.Person from .NET to >>>> the >>>>>>>> Java mapping, >>>>>>>> in the hope that there is a class with that name in Java, which is a >>>>>> very >>>>>>>> rare use case. >>>>>>>> >>>>>>>> [1] >>>>>>>> >>>> org.apache.ignite.internal.MarshallerContextImpl#registerClassName(byte, >>>>>>>> int, java.lang.String, boolean) >>>>>>>> >>>>>>>> On Tue, Jan 12, 2021 at 4:56 PM Nikolay Izhikov < >> nizhi...@apache.org> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> Hello, Igniters. >>>>>>>>> >>>>>>>>> Currently, in case of usage .Net platform client it’s required for >>>> the >>>>>>>>> user to explicitly register each custom type. >>>>>>>>> >>>>>>>>> ``` >>>>>>>>> _marsh = new Marshaller(new BinaryConfiguration >>>>>>>>> { >>>>>>>>> TypeConfigurations = new List<BinaryTypeConfiguration> >>>>>>>>> { >>>>>>>>> new BinaryTypeConfiguration(typeof (Address)), >>>>>>>>> new BinaryTypeConfiguration(typeof (TestModel)) >>>>>>>>> } >>>>>>>>> }); >>>>>>>>> ``` >>>>>>>>> >>>>>>>>> Note, we have a special public interface to map java type to .net >>>>>> types - >>>>>>>>> `IBinaryNameMapper` >>>>>>>>> >>>>>>>>> ``` >>>>>>>>> public interface IBinaryNameMapper >>>>>>>>> { >>>>>>>>> string GetTypeName(string name); >>>>>>>>> string GetFieldName(string name); >>>>>>>>> } >>>>>>>>> } >>>>>>>>> ``` >>>>>>>>> >>>>>>>>> Users found this approach annoying and not convenient. >>>>>>>>> So, recently we made a several improvements for the Service API to >>>>>>>>> implicitly register each custom type from service method arguments. >>>>>> [1], >>>>>>>>> [2], [3] >>>>>>>>> For now, user can just call Java service from .Net without any >>>>>> additional >>>>>>>>> configuration in case FQN NameMapper used. >>>>>>>>> >>>>>>>>> I propose to extends approach with the implicit binary type >>>>>> registration >>>>>>>>> to all APIs and prepare PR for it [4]. >>>>>>>>> >>>>>>>>> What do you think? >>>>>>>>> >>>>>>>>> [1] >>>>>>>>> >>>>>>>> >>>>>> >>>> >> https://github.com/apache/ignite/commit/35f551c023a12b3570d65c803d10c89480f7d5e4 >>>>>>>>> [2] >>>>>>>>> >>>>>>>> >>>>>> >>>> >> https://github.com/apache/ignite/commit/6d02e32e7f049e4f78f7abd37f4ff91d77f738c2 >>>>>>>>> [3] >>>>>>>>> >>>>>>>> >>>>>> >>>> >> https://github.com/apache/ignite/commit/c2204cda29e70294cc93756eabc844b64e07a42e >>>>>>>>> [4] https://github.com/apache/ignite/pull/8635 >>>>>>>> >>>>>> >>>>>> >>>> >>>> >> >>
