[DISCUSS] GraphBinary: Wrap Netty Buffer in public API

[Jorge Bay Gondra <jo...@gmail.com>]

Hi,
As part of GraphBinary implementation, it was decided to use Netty Buffer
API for serialization, which has several benefits over nio
<https://netty.io/wiki/using-as-a-generic-library.html#wiki-h2-0>.

There's a problem with approach taken though, the API of TypeSerializer<T>,
GraphBinaryReader and GraphBinaryWriter exposes Netty's ByteBuf. Exposing
third party library types in an API is generally not a good idea as it
tightly couples your API to the third party library. When considering
dependency versioning and jar shading, this complicates downstream
integration greatly.

I propose we wrap the ByteBuf and expose our own Buffer interface in the
GraphBinary API, that exposes the same readX() and writeX() methods of
ByteBuf.

Thinking one step further, this will allow us to move GraphBinary
serialization to gremlin-core, simplifying dependency management for
integrators.

Introducing Netty's buffer API in our reader/writer APIs was a call I
made... I should have thought of this before doing so. I think there's an
opportunity to fix this in the 3.4 line, as there are no graph providers
that implemented GraphBinary yet. Graph providers implementing it now could
follow the upgrade guide, the good news is that there isn't any user-facing
change expected.

Thoughts?

Jorge

Re: [DISCUSS] GraphBinary: Wrap Netty Buffer in public API

[Stephen Mallette <sp...@gmail.com>]

We tend not to take breaking changes at all on our minor version unless
there is a bug, but if no one is against this change then I won't stand in
the way. Since users mostly interact with the MessageSerializer
implementation which would stay in the driver I guess end-users will likely
not know the difference. The break will mostly be limited to providers who
might be doing stuff with those core classes and upgrade docs should
suffice to deal with that issue i think.

On Mon, Oct 7, 2019 at 10:41 AM Jorge Bay Gondra <jo...@gmail.com>
wrote:

> Thanks for jumping on this discussion. Glad to hear that you agree on the
> general approach.
>
> About the move to gremlin-core in tp34, I see two possibilities:
> a) Copy most of the classes / interfaces in gremlin-core, leave all the
> existing classes gremlin-driver as is and mark them as deprecated (to be
> removed in tp35).
> b) Move the classes / interfaces to gremlin-core
>
> I'm conservative with user-facing changes w/o a deprecation path but given
> that this change only affects graph providers that have implemented
> GraphBinary and there aren't any providers w/ graphbinary in prod yet
> afaik, I think we can get away with it.
>
> Having classes/interfaces in two places with small differences between the
> two can be confusing for devs outside this loop, duplicating code could
> raise the barrier to entry to GraphBinary for implementors. Additionally,
> it would force us to duplicate tests for coverage.
>
> Graph providers can always rely on the upgrade guide if they were
> implementing it and if they haven't yet, when they do, the classes will be
> in the correct place.
>
> wdyt?
>
> On Mon, Oct 7, 2019 at 3:23 PM Stephen Mallette <sp...@gmail.com>
> wrote:
>
> > > I propose we wrap the ByteBuf and expose our own Buffer interface in
> the
> > > GraphBinary API, that exposes the same readX() and writeX() methods of
> > > ByteBuf.
> >
> > +1
> >
> > You address the issue of not breaking APIs really nicely with the
> > following:
> >
> > >  Thinking one step further, this will allow us to move GraphBinary
> > > serialization to gremlin-core, simplifying dependency management for
> > > integrators.
> >
> > I think that we need to leave gremlin-driver code alone in tp34 and
> > deprecate it in favor of the revised approach to the API you've proposed
> in
> > gremlin-core. This also nicely sets up the possibility of using
> GraphBinary
> > beyond the drivers (e.g. OLAP perhaps as a full replacement for Gryo).
> >
> >
> >
> > On Fri, Oct 4, 2019 at 7:49 AM Jorge Bay Gondra <
> jorgebaygondra@gmail.com>
> > wrote:
> >
> > > Hi,
> > > As part of GraphBinary implementation, it was decided to use Netty
> Buffer
> > > API for serialization, which has several benefits over nio
> > > <https://netty.io/wiki/using-as-a-generic-library.html#wiki-h2-0>.
> > >
> > > There's a problem with approach taken though, the API of
> > TypeSerializer<T>,
> > > GraphBinaryReader and GraphBinaryWriter exposes Netty's ByteBuf.
> Exposing
> > > third party library types in an API is generally not a good idea as it
> > > tightly couples your API to the third party library. When considering
> > > dependency versioning and jar shading, this complicates downstream
> > > integration greatly.
> > >
> > > I propose we wrap the ByteBuf and expose our own Buffer interface in
> the
> > > GraphBinary API, that exposes the same readX() and writeX() methods of
> > > ByteBuf.
> > >
> > > Thinking one step further, this will allow us to move GraphBinary
> > > serialization to gremlin-core, simplifying dependency management for
> > > integrators.
> > >
> > > Introducing Netty's buffer API in our reader/writer APIs was a call I
> > > made... I should have thought of this before doing so. I think there's
> an
> > > opportunity to fix this in the 3.4 line, as there are no graph
> providers
> > > that implemented GraphBinary yet. Graph providers implementing it now
> > could
> > > follow the upgrade guide, the good news is that there isn't any
> > user-facing
> > > change expected.
> > >
> > > Thoughts?
> > >
> > > Jorge
> > >
> >
>

Re: [DISCUSS] GraphBinary: Wrap Netty Buffer in public API

[Jorge Bay Gondra <jo...@gmail.com>]

Thanks for jumping on this discussion. Glad to hear that you agree on the
general approach.

About the move to gremlin-core in tp34, I see two possibilities:
a) Copy most of the classes / interfaces in gremlin-core, leave all the
existing classes gremlin-driver as is and mark them as deprecated (to be
removed in tp35).
b) Move the classes / interfaces to gremlin-core

I'm conservative with user-facing changes w/o a deprecation path but given
that this change only affects graph providers that have implemented
GraphBinary and there aren't any providers w/ graphbinary in prod yet
afaik, I think we can get away with it.

Having classes/interfaces in two places with small differences between the
two can be confusing for devs outside this loop, duplicating code could
raise the barrier to entry to GraphBinary for implementors. Additionally,
it would force us to duplicate tests for coverage.

Graph providers can always rely on the upgrade guide if they were
implementing it and if they haven't yet, when they do, the classes will be
in the correct place.

wdyt?

On Mon, Oct 7, 2019 at 3:23 PM Stephen Mallette <sp...@gmail.com>
wrote:

> > I propose we wrap the ByteBuf and expose our own Buffer interface in the
> > GraphBinary API, that exposes the same readX() and writeX() methods of
> > ByteBuf.
>
> +1
>
> You address the issue of not breaking APIs really nicely with the
> following:
>
> >  Thinking one step further, this will allow us to move GraphBinary
> > serialization to gremlin-core, simplifying dependency management for
> > integrators.
>
> I think that we need to leave gremlin-driver code alone in tp34 and
> deprecate it in favor of the revised approach to the API you've proposed in
> gremlin-core. This also nicely sets up the possibility of using GraphBinary
> beyond the drivers (e.g. OLAP perhaps as a full replacement for Gryo).
>
>
>
> On Fri, Oct 4, 2019 at 7:49 AM Jorge Bay Gondra <jo...@gmail.com>
> wrote:
>
> > Hi,
> > As part of GraphBinary implementation, it was decided to use Netty Buffer
> > API for serialization, which has several benefits over nio
> > <https://netty.io/wiki/using-as-a-generic-library.html#wiki-h2-0>.
> >
> > There's a problem with approach taken though, the API of
> TypeSerializer<T>,
> > GraphBinaryReader and GraphBinaryWriter exposes Netty's ByteBuf. Exposing
> > third party library types in an API is generally not a good idea as it
> > tightly couples your API to the third party library. When considering
> > dependency versioning and jar shading, this complicates downstream
> > integration greatly.
> >
> > I propose we wrap the ByteBuf and expose our own Buffer interface in the
> > GraphBinary API, that exposes the same readX() and writeX() methods of
> > ByteBuf.
> >
> > Thinking one step further, this will allow us to move GraphBinary
> > serialization to gremlin-core, simplifying dependency management for
> > integrators.
> >
> > Introducing Netty's buffer API in our reader/writer APIs was a call I
> > made... I should have thought of this before doing so. I think there's an
> > opportunity to fix this in the 3.4 line, as there are no graph providers
> > that implemented GraphBinary yet. Graph providers implementing it now
> could
> > follow the upgrade guide, the good news is that there isn't any
> user-facing
> > change expected.
> >
> > Thoughts?
> >
> > Jorge
> >
>

Re: [DISCUSS] GraphBinary: Wrap Netty Buffer in public API

[Stephen Mallette <sp...@gmail.com>]

> I propose we wrap the ByteBuf and expose our own Buffer interface in the
> GraphBinary API, that exposes the same readX() and writeX() methods of
> ByteBuf.

+1

You address the issue of not breaking APIs really nicely with the following:

>  Thinking one step further, this will allow us to move GraphBinary
> serialization to gremlin-core, simplifying dependency management for
> integrators.

I think that we need to leave gremlin-driver code alone in tp34 and
deprecate it in favor of the revised approach to the API you've proposed in
gremlin-core. This also nicely sets up the possibility of using GraphBinary
beyond the drivers (e.g. OLAP perhaps as a full replacement for Gryo).



On Fri, Oct 4, 2019 at 7:49 AM Jorge Bay Gondra <jo...@gmail.com>
wrote:

> Hi,
> As part of GraphBinary implementation, it was decided to use Netty Buffer
> API for serialization, which has several benefits over nio
> <https://netty.io/wiki/using-as-a-generic-library.html#wiki-h2-0>.
>
> There's a problem with approach taken though, the API of TypeSerializer<T>,
> GraphBinaryReader and GraphBinaryWriter exposes Netty's ByteBuf. Exposing
> third party library types in an API is generally not a good idea as it
> tightly couples your API to the third party library. When considering
> dependency versioning and jar shading, this complicates downstream
> integration greatly.
>
> I propose we wrap the ByteBuf and expose our own Buffer interface in the
> GraphBinary API, that exposes the same readX() and writeX() methods of
> ByteBuf.
>
> Thinking one step further, this will allow us to move GraphBinary
> serialization to gremlin-core, simplifying dependency management for
> integrators.
>
> Introducing Netty's buffer API in our reader/writer APIs was a call I
> made... I should have thought of this before doing so. I think there's an
> opportunity to fix this in the 3.4 line, as there are no graph providers
> that implemented GraphBinary yet. Graph providers implementing it now could
> follow the upgrade guide, the good news is that there isn't any user-facing
> change expected.
>
> Thoughts?
>
> Jorge
>