ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Pavel Tupitsyn <ptupit...@apache.org>
Subject Re: [DISCUSSION] Array to BinaryObject serialization
Date Mon, 03 May 2021 10:03:45 GMT
Nikolay,

I agree that our binary array handling has some limitations - Ignite loses
array element type in many cases (cache.Put -> cache.Get, Binary Mode, etc).

However, for internal platform and services implementations we should fix
the root cause:
avoid extra deserialization->serialization pass completely.
This will also improve performance.

Thanks,
Pavel

On Sat, May 1, 2021 at 9:20 PM Valentin Kulichenko <
valentin.kulichenko@gmail.com> wrote:

> Hi Ivan,
>
> Thanks for chiming in. My comments are below.
>
> -Val
>
> On Sat, May 1, 2021 at 12:20 AM Ivan Daschinsky <ivandasch@gmail.com>
> wrote:
>
> > Hi!
> > First of all, when array is serialized, marshaller actually DO
> > PRESERVE type of element (seel
> > org.apache.ignite.internal.binary.BinaryUtils#doReadObjectArray and
> > org.apache.ignite.internal.binary.BinaryWriterExImpl#doWriteObjectArray).
> > AFAIK, the motivation of Nickolay proposal, is the fact, that during
> > call of PlatformService we do additional marshal/unmarshall step and
> > during this step we loose array type info
> > See
> org.apache.ignite.internal.binary.BinaryReaderExImpl#readObjectDetached
> > and org.apache.ignite.internal.binary.BinaryUtils#doReadObjectArray)
> >
> > In order to handle this problem, we can just add some wrapper that
> > contains element type info and use it in our INTERNAL API.
> > I just don't understand why we should change IgniteBinary API.
> >
>
> Makes sense to me. I would also avoid changing the public API.
>
>
> >
> > >>  It was designed as a data storage format, and the fact
> > that Ignite uses it elsewhere is a fundamental design flaw. BTW, this is
> > something we're trying to fix in 3.0.
> >
> > Please, do not. There are many cases when this can really improve
> > performance. Reflection is always slower than low level api and many
> > users are happy with low level API.
> >
>
> Low-level APIs are not being removed. If anything, they are likely to
> become more low-level :) Either way, that's off-topic. I would recommend
> reviewing the related IEPs and starting a separate discussion if you have
> any questions or concerns.
>
>
> >
> > сб, 1 мая 2021 г. в 00:51, Valentin Kulichenko <
> > valentin.kulichenko@gmail.com>:
> > >
> > > Hi Nikolay,
> > >
> > > Is there a specific motivation behind your proposal? I do acknowledge
> > that
> > > the semantics of the toBinary method is a little weird, but my concern
> is
> > > that the way it works with arrays is just an example. Are you
> suggesting
> > > changing collections, primitives, and other "first citizen" data types
> as
> > > well? To me, that looks like a huge risky change without a clear value.
> > >
> > > I actually believe that binary format *should not* be used as a
> universal
> > > serde mechanism. It was designed as a data storage format, and the fact
> > > that Ignite uses it elsewhere is a fundamental design flaw. BTW, this
> is
> > > something we're trying to fix in 3.0.
> > >
> > > That said, I'm not necessarily against the change (especially if we do
> > not
> > > change the default behavior). But I would like to better understand its
> > > scope (e.g., arrays only or beyond?), as well as get some examples of
> use
> > > cases that would benefit from the change.
> > >
> > > -Val
> > >
> > >
> > > On Fri, Apr 30, 2021 at 7:35 AM Nikolay Izhikov <nizhikov@apache.org>
> > wrote:
> > >
> > > > 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.kasnacheev@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 <nizhikov@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.dev@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
> > > > >>
> > > > >>
> > > >
> > > >
> >
> >
> >
> > --
> > Sincerely yours, Ivan Daschinskiy
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message