Github user dalaro commented on the issue:

    https://github.com/apache/incubator-tinkerpop/pull/325
  
    @spmallette You're right about overrides being the problem.
    
    The last thing I did before opening this PR was to rebase on latest master, 
when I encountered conflicts solely involving the GryoMapper registration 
override feature added in d7684757734732f39970265a425b921a48d8f0fb.  I 
attempted to resolve those conflicts too hastily and got it wrong.
    
    I went back to Stephen's original commit reworked my code around 
registration overrides, and I now think it conforms to the old behavior 
precedent.  Specifically, when handling overrides of GryoMapper type 
registrations, I departed from the precedent set by Stephen's commit in two 
ways, both of which I've fixed locally:
    
    * Override registrations allocated a new registration ID.  Your commit used 
the old ID from the registration being overridden.
    * As a side effect of the preceding point, I incremented the shared 
`currentSerializationId` for all registration calls.  Your commit incremented 
this shared counter only for non-override registration calls.
    
    The test passes locally for me now.  I'm still doing some registrator 
refactoring and haven't pushed yet though.
    
    @okram The problem with custom registrations is that GryoMapper allows the 
user to provide a serializer written against shaded Kryo.  This is not 
compatible with Spark's KryoSerializer.  I don't see a way to make it 
compatible without putting fake `org.apache.tinkerpop.shaded.kryo.*` classes on 
the classpath, which could get pretty ugly.
    
    Instead, I'm adding a registrator that includes:
    
    * all default mappings from GryoMapper (I am changing the 
shaded-Kryo-serializers to be shim serializers so that they are reusable)
    * the TinkerPop classes registered in GryoSerializer's constructors (but 
not the scala and Spark classes registered in there)
    
    If the user wants to register custom types while using KryoSerializer, they 
can extend the registrator in a subclass, then set `spark.kryo.registrator` to 
the subclass.  The interface in question is `KryoRegistrator` and it has only 
one method, `registerClasses(Kryo)`.  It's about as simple -- maybe even less 
so -- than implementing TP's `IoRegistry` interface, which has two methods.
    
    If the user decides to continue using GryoSerializer, they can also 
continue using `GryoPool.CONFIG_IO_REGISTRY`, which should still affect 
GryoSerializer/HadoopPools as it always has.
    
    WDYT?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Reply via email to