Uploaded image for project: 'TinkerPop'
  1. TinkerPop
  2. TINKERPOP-1402

Impossible for graph implementations to provide a class resolver for Gryo IO

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Critical
    • Resolution: Done
    • Affects Version/s: 3.2.1
    • Fix Version/s: 3.2.2
    • Component/s: structure
    • Labels:
      None

      Description

      As far as I can tell there is no way for a graph implementation to specify a classresolver for the following code:

      g.io(IoCore.gryo()).writer().create()

      The problem is that inside the graph implementation we need to be able to do this:

      public <I extends Io> I io(final Io.Builder<I> builder) {
        Io io = builder.graph(this).registry(MyRegistry.INSTANCE).classResolver(MyClassReolver.INSTANCE).create();
      

      but only supplying a registry is supported.
      Other solutions could be to design GryoIo for extension so that it can be wrapped or to change the signature of Graph#io to:

      public default Io io(final Io.Builder<I> builder)
      

      I would probably go for the signature change, so the graph is responsible for deciding the implementation that is returned.

        Issue Links

          Activity

          Hide
          spmallette stephen mallette added a comment -

          I'm hesitant to introduce a breaking change to the structure API, but I see the problem (i'm also not sure I see how changing the signature helps - a graph would get one type of Builder and then just sorta throw it away for whatever Io instance it feels like returning? maybe i'm missing the point). The classResolver is something newer that didn't really have a place in the Io.Builder interface, especially since it's specific to Gryo and really doesn't have a similar concept anywhere else.

          I'm wondering if a quieter way to handle it would be to add another method to Io.Builder:

          public Builder<? extends Io> onMapper(final UnaryOperator<Mapper.Builder> mapper);
          

          Of course, that makes the point of the registry method a bit useless as then you could just assign the IoRegistry in the UnaryOperator. Maybe the IoRegistry could somehow have some function in allowing a graph to register a ClassResolver there in a general way?

          Show
          spmallette stephen mallette added a comment - I'm hesitant to introduce a breaking change to the structure API, but I see the problem (i'm also not sure I see how changing the signature helps - a graph would get one type of Builder and then just sorta throw it away for whatever Io instance it feels like returning? maybe i'm missing the point). The classResolver is something newer that didn't really have a place in the Io.Builder interface, especially since it's specific to Gryo and really doesn't have a similar concept anywhere else. I'm wondering if a quieter way to handle it would be to add another method to Io.Builder : public Builder<? extends Io> onMapper( final UnaryOperator<Mapper.Builder> mapper); Of course, that makes the point of the registry method a bit useless as then you could just assign the IoRegistry in the UnaryOperator . Maybe the IoRegistry could somehow have some function in allowing a graph to register a ClassResolver there in a general way?
          Hide
          bryncooke Bryn Cooke added a comment - - edited

          I like the onMapper idea and also agree the registry method is redundant.
          Trying to unify the serialization apis would seem to be a pretty tall order, onMapper gives implementations the required flexibility. I think that the registry method should be deprecated.

          Show
          bryncooke Bryn Cooke added a comment - - edited I like the onMapper idea and also agree the registry method is redundant. Trying to unify the serialization apis would seem to be a pretty tall order, onMapper gives implementations the required flexibility. I think that the registry method should be deprecated.
          Hide
          spmallette stephen mallette added a comment -

          OK - I'd like to think on it for today but if I don't think of anything better I'll suggest deprecation of registry on the mailing list in favor of onMapper (or a better name) and see if anyone has any issues.

          Show
          spmallette stephen mallette added a comment - OK - I'd like to think on it for today but if I don't think of anything better I'll suggest deprecation of registry on the mailing list in favor of onMapper (or a better name) and see if anyone has any issues.
          Hide
          bryncooke Bryn Cooke added a comment -

          On reflection I'd use Consumer rather than UnaryOperator. We won't ever return a new builder, just call methods on the one we get passed.

          Show
          bryncooke Bryn Cooke added a comment - On reflection I'd use Consumer rather than UnaryOperator. We won't ever return a new builder, just call methods on the one we get passed.
          Hide
          spmallette stephen mallette added a comment -

          +1 - just sent a DISCUSS thread referenced as a link on this ticket

          Show
          spmallette stephen mallette added a comment - +1 - just sent a DISCUSS thread referenced as a link on this ticket
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user spmallette opened a pull request:

          https://github.com/apache/tinkerpop/pull/380

          TINKERPOP-1402 Added IoBuilder.onMapper() method.

          https://issues.apache.org/jira/browse/TINKERPOP-1402

          Deprecated IoBuilder.registry() in the process of doing that. This should provide greater flexibility to Graph providers who can now not only supply a custom IoRegistry but also access other non-generalized features of a Mapper.Builder instance.

          Tested with `mvn clean install`. I did a one-time test before changing TinkerGraph to stop using the deprecated `registry` method and the build worked fine so I think that even though we can't directly test the old `registry` method anymore, it still should be working ok if provides elect to stick with it.

          VOTE +1

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/apache/tinkerpop TINKERPOP-1402

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/tinkerpop/pull/380.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #380


          commit f1617e202025db1ff6f94deb70c66f14772273bf
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2016-08-15T14:29:46Z

          Added IoBuilder.onMapper() method.

          Deprecated IoBuilder.registry() in the process of doing that. This should provide greater flexibility to Graph providers who can now not only supply a custom IoRegistry but also access other non-generalized features of a Mapper.Builder instance.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user spmallette opened a pull request: https://github.com/apache/tinkerpop/pull/380 TINKERPOP-1402 Added IoBuilder.onMapper() method. https://issues.apache.org/jira/browse/TINKERPOP-1402 Deprecated IoBuilder.registry() in the process of doing that. This should provide greater flexibility to Graph providers who can now not only supply a custom IoRegistry but also access other non-generalized features of a Mapper.Builder instance. Tested with `mvn clean install`. I did a one-time test before changing TinkerGraph to stop using the deprecated `registry` method and the build worked fine so I think that even though we can't directly test the old `registry` method anymore, it still should be working ok if provides elect to stick with it. VOTE +1 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/tinkerpop TINKERPOP-1402 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/380.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #380 commit f1617e202025db1ff6f94deb70c66f14772273bf Author: Stephen Mallette <spmva@genoprime.com> Date: 2016-08-15T14:29:46Z Added IoBuilder.onMapper() method. Deprecated IoBuilder.registry() in the process of doing that. This should provide greater flexibility to Graph providers who can now not only supply a custom IoRegistry but also access other non-generalized features of a Mapper.Builder instance.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/380

          Note that the build failure in travis is that `IdentityRemovalStrategyTest$PerformanceTest>TraversalStrategyPerformanceTest.shouldBeFaster:118 null` nonsense

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/380 Note that the build failure in travis is that `IdentityRemovalStrategyTest$PerformanceTest>TraversalStrategyPerformanceTest.shouldBeFaster:118 null` nonsense
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

          https://github.com/apache/tinkerpop/pull/380

          VOTE: +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/380 VOTE: +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user twilmes commented on the issue:

          https://github.com/apache/tinkerpop/pull/380

          VOTE: +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user twilmes commented on the issue: https://github.com/apache/tinkerpop/pull/380 VOTE: +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/tinkerpop/pull/380

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tinkerpop/pull/380

            People

            • Assignee:
              spmallette stephen mallette
              Reporter:
              bryncooke Bryn Cooke
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development