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 <[email protected]> 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 <[email protected]> > 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 > > >
