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 >>>>>> >>>> >>>> >> >>
