Hello, Igniters. I've prepared implementation of support of BinaryArray. Please, join the review.
Ticket - https://issues.apache.org/jira/browse/IGNITE-14742 PR - https://github.com/apache/ignite/pull/9490 ср, 19 мая 2021 г. в 14:44, Ilya Kasnacheev <ilya.kasnach...@gmail.com>: > 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 > > >>>>>>>>>>> > > >>>>>>>>>>> > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>> > > >>>>>>> > > >>>>> > > >>>> > > >>>> > > >> > > >> > > > > >
