Hello! We don't have to do it in toBinary() - we may preserve its behavior if you like, and only do that transition for Cache API, where additional support may be added to e.g. org.apache.ignite.internal.processors.cache.binary.CacheObjectBinaryProcessorImpl
Regards, -- Ilya Kasnacheev ср, 19 мая 2021 г. в 14:23, Nikolay Izhikov <nizhi...@apache.org>: > Ilya. > > Actually, current behaviour described even in documentation [1] > > > > Note that not all objects are converted to the binary object format. The > following classes are never converted (e.g., the toBinary(Object) method > returns the original object, and instances of these classes are stored > without changes): > ... > > ... arrays of objects (but the objects inside them are reconverted if > they are binary) > > [1] > https://ignite.apache.org/docs/latest/key-value-api/binary-objects#enabling-binary-mode-for-caches > > > 19 мая 2021 г., в 14:12, Ilya Kasnacheev <ilya.kasnach...@gmail.com> > написал(а): > > > > Hello! > > > > Why do you need to take compatibility into account here at all? > > You can start writing entries to cache in new format while reading both > old > > and new format. > > > > I consider returning Object[] instead of ConcreteType[] a bug so it's > > totally OK to start returning the "better" ConcreteType[] instead. > > > > I think you can just fix the bug while preserving accessibility of old > > (pre-bugfix) data as it was pre-2.11. > > > > Regards, > > -- > > Ilya Kasnacheev > > > > > > ср, 19 мая 2021 г. в 14:08, Nikolay Izhikov <nizhi...@apache.org>: > > > >> Ilya, > >> > >>> Maybe we should just automate that, e.g., whenever user stores Type[], > >> always store one-field ArrayWrapper object, and automatically unwrap it > on > >> get() > >> > >> Yes, I’m talking about this opportunity from the beginning of the > thread. > >> > >>> 1. Implement binary serialization that correctly Ser and Deser array > >> using some kind of the wrapper (BinaryArrayWrapper). > >> > >> > >>> 19 мая 2021 г., в 14:05, Ilya Kasnacheev <ilya.kasnach...@gmail.com> > >> написал(а): > >>> > >>> Hello! > >>> > >>> Yes, it does not look pretty, I agree. But I'm saying that I've not > >>> encountered this issue on the user list before, and it can probably be > >>> easily countered by storing some one-field ArrayWrapper object in the > >> cache > >>> as value. > >>> > >>> Maybe we should just automate that, e.g., whenever user stores Type[], > >>> always store one-field ArrayWrapper object, and automatically unwrap it > >> on > >>> get() > >>> This way we may not even need any changes to storage format, if binary > >>> marshaller does not mind. > >>> > >>> Regards, > >>> > >>> Regards, > >>> -- > >>> Ilya Kasnacheev > >>> > >>> > >>> ср, 19 мая 2021 г. в 13:33, Nikolay Izhikov <nizhi...@apache.org>: > >>> > >>>> Igniters. > >>>> > >>>> Just to clarify the issue: > >>>> > >>>> ``` > >>>> public class BinaryObjectTest extends GridCommonAbstractTest { > >>>> /** */ > >>>> @Test > >>>> public void testArray() throws Exception { > >>>> Ignite ign = startGrid(); > >>>> > >>>> IgniteCache<Integer, TestClass1[]> cache = > >>>> ign.createCache("my-cache"); > >>>> > >>>> cache.put(1, new TestClass1[] {new TestClass1(), new > >>>> TestClass1()}); > >>>> TestClass1[] obj = cache.get(1); //This will fail with > >>>> ClassCastException. > >>>> > >>>> assertEquals(TestClass1[].class, obj.getClass()); > >>>> } > >>>> } > >>>> ``` > >>>> > >>>>> 19 мая 2021 г., в 13:04, Nikolay Izhikov <nizhikov....@gmail.com> > >>>> написал(а): > >>>>> > >>>>> Thanks, Ilya. > >>>>> > >>>>> Can you put more context on this? > >>>>> I don’t familiar with these issues. > >>>>> > >>>>>> 19 мая 2021 г., в 13:02, Ilya Kasnacheev <ilya.kasnach...@gmail.com > > > >>>> написал(а): > >>>>>> > >>>>>> Hello! > >>>>>> > >>>>>> Obvious issues are Lazy SQL, Event Driven Services, Sort Binary > Object > >>>>>> Fields. > >>>>>> > >>>>>> Regards, > >>>>>> -- > >>>>>> Ilya Kasnacheev > >>>>>> > >>>>>> > >>>>>> ср, 19 мая 2021 г. в 12:56, Nikolay Izhikov <nizhi...@apache.org>: > >>>>>> > >>>>>>> Hello, > >>>>>>> > >>>>>>>> However, for internal platform and services implementations we > >> should > >>>>>>> fix the root cause: > >>>>>>>> avoid extra deserialization->serialization pass completely. > >>>>>>>> This will also improve performance. > >>>>>>> > >>>>>>> Pavel, thanks for the feedback. > >>>>>>> If I understand correctly, your suggestion is to know data size at > >> the > >>>>>>> start of reading service parameters. > >>>>>>> Is it correct? > >>>>>>> > >>>>>>> Right now, when the service method invoked we pass an array of > >>>> parameters > >>>>>>> through platform reader/writer machinery. > >>>>>>> On java side parameters read one by one and we don’t know the size > of > >>>> the > >>>>>>> data on the read start. > >>>>>>> > >>>>>>> AFAICU, this will require x2 memory on the .Net or thin > client-side. > >>>>>>> > >>>>>>> > >>>>>>> > >>>> > >> > https://github.com/apache/ignite/blob/master/modules/core/src/main/java/org/apache/ignite/internal/processors/platform/services/PlatformServices.java#L289 > >>>>>>> > >>>>>>> > >>>>>>>> if we are to break compatibility, I would like to see it done for > >> some > >>>>>>> really common pain point. > >>>>>>> > >>>>>>> Ilya, can you, please, provide a list of common issues with Ignite > >> that > >>>>>>> can be resolved > >>>>>>> only with compatibility breakage? > >>>>>>> > >>>>>>>> 4 мая 2021 г., в 12:58, Ilya Kasnacheev < > ilya.kasnach...@gmail.com> > >>>>>>> написал(а): > >>>>>>>> > >>>>>>>> Hello! > >>>>>>>> > >>>>>>>> If we really decide to break some compatibility then Array to > >>>>>>> BinaryObject > >>>>>>>> serialization will be very, very low on my personal list. > >>>>>>>> > >>>>>>>> I just don't see how this issue is relevant. I have been reading > and > >>>>>>>> answering user list for a few years now, and I don't remember a > >> single > >>>>>>>> question about storing ConcreteType[] as value and complaining > about > >>>> type > >>>>>>>> information loss. > >>>>>>>> > >>>>>>>> If you have a good scenario how do we keep persistent store binary > >>>>>>>> compatibility here, without adding a lot of new code and still > >>>> checking > >>>>>>> for > >>>>>>>> both old and new approaches - you can go forward for sure. > >>>>>>>> > >>>>>>>> However, it does seem questionable where we have a new wrapper > class > >>>>>>>> specifically for top level arrays. You can have this wrapper in > your > >>>> own > >>>>>>>> client code and it should work OK. > >>>>>>>> > >>>>>>>> Bottom line, if we are to break compatibility, I would like to see > >> it > >>>>>>> done > >>>>>>>> for some really common pain point. > >>>>>>>> > >>>>>>>> Regards, > >>>>>>>> -- > >>>>>>>> Ilya Kasnacheev > >>>>>>>> > >>>>>>>> > >>>>>>>> пт, 30 апр. 2021 г. в 17:34, Nikolay Izhikov <nizhi...@apache.org > >: > >>>>>>>> > >>>>>>>>> Hello, Ilya. > >>>>>>>>> > >>>>>>>>> Thanks for the feedback! > >>>>>>>>> > >>>>>>>>>> For me it sounds like something we would like to do in 3.0 > >>>>>>>>> > >>>>>>>>> Ignite 3 is a very long way to go, so I prefer to target this fix > >> in > >>>>>>>>> Ignite 2.x. > >>>>>>>>> > >>>>>>>>>> I think making it default "true" is a breaking change and is not > >>>>>>>>> possible in a minor release > >>>>>>>>> > >>>>>>>>> Yes, you are correct it is a breaking change. > >>>>>>>>> It seems for me, we all agreed that breaking changes are possible > >> in > >>>>>>> minor > >>>>>>>>> releases. > >>>>>>>>> > >>>>>>>>> Anyway, if we will decide do not to enable this feature by > default > >>>> it’s > >>>>>>> OK > >>>>>>>>> for me. > >>>>>>>>> We still can implement it and improve the binary SerDe mechanism. > >>>>>>>>> > >>>>>>>>> > >>>>>>>>>> 30 апр. 2021 г., в 17:23, Ilya Kasnacheev < > >>>> ilya.kasnach...@gmail.com> > >>>>>>>>> написал(а): > >>>>>>>>>> > >>>>>>>>>> Hello! > >>>>>>>>>> > >>>>>>>>>> For me it sounds like something we would like to do in 3.0 (if > >>>> indeed > >>>>>>> it > >>>>>>>>>> will have arrays as possible value (or key) type), but doing it > in > >>>> 2.x > >>>>>>>>>> raises concerns whether it has enough time left to stabilize. > >>>>>>>>>> > >>>>>>>>>> Also, I think making it default "true" is a breaking change and > is > >>>> not > >>>>>>>>>> possible in a minor release, case in point, > >>>>>>>>>> IGNITE_BINARY_SORT_OBJECT_FIELDS is still waiting for 3.0 and it > >> is > >>>>>>> less > >>>>>>>>>> destructive. > >>>>>>>>>> > >>>>>>>>>> Of course I would also like to hear what other community members > >>>> think. > >>>>>>>>>> > >>>>>>>>>> Regards, > >>>>>>>>>> -- > >>>>>>>>>> Ilya Kasnacheev > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> пт, 30 апр. 2021 г. в 17:16, Nikolay Izhikov < > nizhi...@apache.org > >>> : > >>>>>>>>>> > >>>>>>>>>>> Igniters, > >>>>>>>>>>> > >>>>>>>>>>> Want to clarify my proposal about new array store format. > >>>>>>>>>>> I think we should store array in special binary wrapper that > will > >>>> keep > >>>>>>>>>>> original component type > >>>>>>>>>>> > >>>>>>>>>>> ``` > >>>>>>>>>>> public class BinaryArrayWrapper implements BinaryObjectEx, > >>>>>>>>> Externalizable { > >>>>>>>>>>> /** Type ID. */ > >>>>>>>>>>> private int compTypeId; > >>>>>>>>>>> > >>>>>>>>>>> /** Raw data. */ > >>>>>>>>>>> private String compClsName; > >>>>>>>>>>> > >>>>>>>>>>> /** Value. */ > >>>>>>>>>>> private Object[] arr; > >>>>>>>>>>> > >>>>>>>>>>> // Further implementation. > >>>>>>>>>>> } > >>>>>>>>>>> ``` > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>>> 30 апр. 2021 г., в 16:31, Nikolay Izhikov < > >> nizhikov....@gmail.com > >>>>> > >>>>>>>>>>> написал(а): > >>>>>>>>>>>> > >>>>>>>>>>>> Hello, Igniters. > >>>>>>>>>>>> > >>>>>>>>>>>> Currently, binary marshaller works as follows(Say, we have a > >> class > >>>>>>>>>>> `User` then): > >>>>>>>>>>>> > >>>>>>>>>>>> IgniteBinary#toBinary(User)` -> BinaryObject > >>>>>>>>>>>> IgniteBinary#toBinary(User[])` -> Object[] > >>>>>>>>>>>> IgniteBinary#toBinary(Object[])` -> Object[] > >>>>>>>>>>>> > >>>>>>>>>>>> This means, that we lose array component type information > during > >>>>>>> binary > >>>>>>>>>>> serialization. > >>>>>>>>>>>> AFAIK, it’s a design choice made during binary infrastructure > >>>>>>>>>>> development. > >>>>>>>>>>>> > >>>>>>>>>>>> This lead to the following disadvantages: > >>>>>>>>>>>> > >>>>>>>>>>>> 1. `IgniteBinary` can’t be used as a universal SerDe > mechanism. > >>>>>>>>>>>> 2. Ignite internals(service grid, .Net calls) contains many > >> tweaks > >>>>>>> and > >>>>>>>>>>> hacks to deal with custom user array and still has many issues > >> [1] > >>>>>>>>>>>> > >>>>>>>>>>>> I propose to make breaking changes and fix the custom user > array > >>>> SeDe > >>>>>>>>> as > >>>>>>>>>>> follows: > >>>>>>>>>>>> > >>>>>>>>>>>> 1. Implement binary serialization that correctly Ser and Deser > >>>>>>>>>>> array using some kind of the wrapper (BinaryArrayWrapper). > >>>>>>>>>>>> > >>>>>>>>>>>> IgniteBinary#toBinary(User)` -> BinaryObject > >>>>>>>>>>>> IgniteBinary#toBinary(User[])` -> BinaryObject > >>>>>>>>>>>> IgniteBinary#toBinary(Object[])` -> BinaryObject > >>>>>>>>>>>> > >>>>>>>>>>>> 2. Introduce system flag `IGNITE_USE_BINARY_ARRAY` that > enables > >>>>>>>>>>> correct SerDe of arrays. The default value is false to keep > >>>> backward > >>>>>>>>>>> compatibility in the next Ignite release(2.11). > >>>>>>>>>>>> > >>>>>>>>>>>> 3. Set `IGNITE_USE_BINARY_ARRAY` to `true` in the ongoing > >>>> Ignite > >>>>>>>>>>> releases (2.12). > >>>>>>>>>>>> > >>>>>>>>>>>> WDYT? > >>>>>>>>>>>> > >>>>>>>>>>>> [1] https://issues.apache.org/jira/browse/IGNITE-14299 > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>> > >>>>>>> > >>>>> > >>>> > >>>> > >> > >> > >
