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

Allow any GraphReader/Writer to be persistence engine for TinkerGraph

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Trivial
    • Resolution: Implemented
    • Affects Version/s: 3.0.2-incubating
    • Fix Version/s: 3.1.1-incubating
    • Component/s: tinkergraph
    • Labels:
      None

      Description

      TinkerGraph currently works with gryo, graphml and graphson - all internal formats to TinkerPop. This could easily be extended to work with any format implementing the appropriate interfaces (i.e. also external third-party formats) by allowing for the `gremlin.tinkergraph.graphFormat` setting for TinkerGraph to be set to the existing three settings, but also allow for it to be the fully qualified class name for a Io.Builder<I extends Io> interface. TinkerGraph could then dynamically instantiate this class (which we can expect to have a zero-arg constructor) and use it to load/save data. This might actually also clean up some of the existing code around load/save.

        Activity

        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user spmallette commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-159111306

        @kushal256 i've merged this one. thanks for working through all that. note that were some weird file permission changes that came with your PR. I reverted those:

        https://github.com/apache/incubator-tinkerpop/commit/5f1a354e577b7c31f00469c91acae61a6144a85f

        for some reason, all those files were made executable. ??? anyway, something to watch for your next PR. thanks again!

        Show
        githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-159111306 @kushal256 i've merged this one. thanks for working through all that. note that were some weird file permission changes that came with your PR. I reverted those: https://github.com/apache/incubator-tinkerpop/commit/5f1a354e577b7c31f00469c91acae61a6144a85f for some reason, all those files were made executable. ??? anyway, something to watch for your next PR. thanks again!
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

        https://github.com/apache/incubator-tinkerpop/pull/141

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

        Github user kushal256 commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158981359

        @spmallette, yes, I made the changes as per your recommendation, ready for
        merge.

        @kushal256 <https://github.com/kushal256> i see you made some changes - is
        this ready for me to merge now?


        Reply to this email directly or view it on GitHub
        <https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158980645>
        .

        Show
        githubbot ASF GitHub Bot added a comment - Github user kushal256 commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158981359 @spmallette, yes, I made the changes as per your recommendation, ready for merge. @kushal256 < https://github.com/kushal256 > i see you made some changes - is this ready for me to merge now? — Reply to this email directly or view it on GitHub < https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158980645 > .
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user spmallette commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158980645

        @kushal256 i see you made some changes - is this ready for me to merge now?

        Show
        githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158980645 @kushal256 i see you made some changes - is this ready for me to merge now?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user spmallette commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158584247

        @kushal256 not a problem - i figured your IDE was the culprit. if you'd like to amend your PR with the fix to the imports that will be fine.

        The other thing we tend to do is mark all variables `final` (unless they are not - which really makes them jump out at you and makes you wonder why they are not when you see them in the code base). so for example you added:

        ```text
        public static Io.Builder createIoBuilder(String graphFormat) throws ClassNotFoundException, IllegalAccessException, InstantiationException

        { Class<Io.Builder> ioBuilderClass = (Class<Io.Builder>) Class.forName(graphFormat); Io.Builder ioBuilder = ioBuilderClass.newInstance(); return ioBuilder; }

        ```

        `graphFormat`, `ioBuilder` and `ioBuilderClass` should be `final`. `IoCore` is also a "public" API class, so some javadoc on that method would be good.

        And last but not least, if you want to really tie a bow on this, you would add an entry to the CHANGELOG for 3.1.1:

        https://github.com/apache/incubator-tinkerpop/blob/master/CHANGELOG.asciidoc#tinkerpop-311-not-officially-released-yet

        Please comment when you've updated the PR with those little changes and i'll merge after that.

        Show
        githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158584247 @kushal256 not a problem - i figured your IDE was the culprit. if you'd like to amend your PR with the fix to the imports that will be fine. The other thing we tend to do is mark all variables `final` (unless they are not - which really makes them jump out at you and makes you wonder why they are not when you see them in the code base). so for example you added: ```text public static Io.Builder createIoBuilder(String graphFormat) throws ClassNotFoundException, IllegalAccessException, InstantiationException { Class<Io.Builder> ioBuilderClass = (Class<Io.Builder>) Class.forName(graphFormat); Io.Builder ioBuilder = ioBuilderClass.newInstance(); return ioBuilder; } ``` `graphFormat`, `ioBuilder` and `ioBuilderClass` should be `final`. `IoCore` is also a "public" API class, so some javadoc on that method would be good. And last but not least, if you want to really tie a bow on this, you would add an entry to the CHANGELOG for 3.1.1: https://github.com/apache/incubator-tinkerpop/blob/master/CHANGELOG.asciidoc#tinkerpop-311-not-officially-released-yet Please comment when you've updated the PR with those little changes and i'll merge after that.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user kushal256 commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158556607

        Awesome! Apologies for the import with a wildcard, my Intellij did that
        for me, I can clean that up, let me know if there are any additional
        cleanup items, want to ensure everything meets the project's standards.

        On Fri, Nov 20, 2015 at 1:43 PM, Daniel Kuppitz <notifications@github.com>
        wrote:

        > I've been almost happy with the first version and now that something has
        > been added to the docs:
        >
        > VOTE: +1
        >
        > —
        > Reply to this email directly or view it on GitHub
        > <https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158532860>
        > .
        >

        Show
        githubbot ASF GitHub Bot added a comment - Github user kushal256 commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158556607 Awesome! Apologies for the import with a wildcard, my Intellij did that for me, I can clean that up, let me know if there are any additional cleanup items, want to ensure everything meets the project's standards. On Fri, Nov 20, 2015 at 1:43 PM, Daniel Kuppitz <notifications@github.com> wrote: > I've been almost happy with the first version and now that something has > been added to the docs: > > VOTE: +1 > > — > Reply to this email directly or view it on GitHub > < https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158532860 > > . >
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user dkuppitz commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158532860

        I've been almost happy with the first version and now that something has been added to the docs:

        VOTE: +1

        Show
        githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158532860 I've been almost happy with the first version and now that something has been added to the docs: VOTE: +1
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user okram commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158526811

        I pulled the branch and `mvn clean install`'d. Everything passed. I also reviewed the code. I don't really know this area of the codebase but the code is in our style and Stephen agrees with the PR, so thats good enough for me.

        VOTE +1

        Show
        githubbot ASF GitHub Bot added a comment - Github user okram commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158526811 I pulled the branch and `mvn clean install`'d. Everything passed. I also reviewed the code. I don't really know this area of the codebase but the code is in our style and Stephen agrees with the PR, so thats good enough for me. VOTE +1
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user twilmes commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158146890

        Looks good to me. Ran the tests and then played around with it in the shell a bit. Exception message is good on a bogus persistence engine and gryo and graphson work as expected.

        +1

        Show
        githubbot ASF GitHub Bot added a comment - Github user twilmes commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158146890 Looks good to me. Ran the tests and then played around with it in the shell a bit. Exception message is good on a bogus persistence engine and gryo and graphson work as expected. +1
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user spmallette commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158053106

        I think we're good to VOTE on this now. Ran `mvn clean install` and all tests pass. There's a few little odds/ends I'll cleanup after merge.

        +1

        Show
        githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158053106 I think we're good to VOTE on this now. Ran `mvn clean install` and all tests pass. There's a few little odds/ends I'll cleanup after merge. +1
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user spmallette commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-157080928

        thanks for updating the PR. we're in the midst of releasing 3.1.0-incubating. we'll review this in more detail as soon as that is out the door.

        Show
        githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-157080928 thanks for updating the PR. we're in the midst of releasing 3.1.0-incubating. we'll review this in more detail as soon as that is out the door.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user spmallette commented on a diff in the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/141#discussion_r44941599

        — Diff: tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphTest.java —
        @@ -23,19 +23,21 @@
        import org.apache.tinkerpop.gremlin.TestHelper;
        import org.apache.tinkerpop.gremlin.process.traversal.P;
        import org.apache.tinkerpop.gremlin.structure.Edge;
        +import org.apache.tinkerpop.gremlin.structure.Graph;
        import org.apache.tinkerpop.gremlin.structure.Vertex;
        +import org.apache.tinkerpop.gremlin.structure.io.Io;
        import org.apache.tinkerpop.gremlin.structure.io.IoCore;
        +import org.apache.tinkerpop.gremlin.structure.io.IoRegistry;
        import org.apache.tinkerpop.gremlin.structure.io.IoTest;
        import org.junit.Ignore;
        import org.junit.Test;

        -import java.io.ByteArrayInputStream;
        -import java.io.ByteArrayOutputStream;
        -import java.io.File;
        +import java.io.*;
        — End diff –

        not a big deal, but for future pull requests, note that for style purposes we don't use wildcard imports.

        Show
        githubbot ASF GitHub Bot added a comment - Github user spmallette commented on a diff in the pull request: https://github.com/apache/incubator-tinkerpop/pull/141#discussion_r44941599 — Diff: tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphTest.java — @@ -23,19 +23,21 @@ import org.apache.tinkerpop.gremlin.TestHelper; import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.structure.Edge; +import org.apache.tinkerpop.gremlin.structure.Graph; import org.apache.tinkerpop.gremlin.structure.Vertex; +import org.apache.tinkerpop.gremlin.structure.io.Io; import org.apache.tinkerpop.gremlin.structure.io.IoCore; +import org.apache.tinkerpop.gremlin.structure.io.IoRegistry; import org.apache.tinkerpop.gremlin.structure.io.IoTest; import org.junit.Ignore; import org.junit.Test; -import java.io.ByteArrayInputStream; -import java.io.ByteArrayOutputStream; -import java.io.File; +import java.io.*; — End diff – not a big deal, but for future pull requests, note that for style purposes we don't use wildcard imports.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user kushal256 commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-155627962

        OK just checked in with an old fashioned mock.

        Show
        githubbot ASF GitHub Bot added a comment - Github user kushal256 commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-155627962 OK just checked in with an old fashioned mock.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user spmallette commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-155586988

        Don't use mockito. Create your mock the old fashioned way and just create a concrete class in test that implements the interfaces you need.

        Show
        githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-155586988 Don't use mockito. Create your mock the old fashioned way and just create a concrete class in test that implements the interfaces you need.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user kushal256 commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-155581913

        Thanks for your comments. Yes, there should be similar logic for writing
        graph.

        Also, I agree, there should be tests, but I'm not sure how to do so when
        using reflection (Class.forName() and newInstance() for creating new
        Io.Builder), with mockito's stubs. Any recommendations for how to do this?
        On Nov 9, 2015 4:13 AM, "Daniel Kuppitz" <notifications@github.com> wrote:

        > I agree, I'd also like to see that covered by one or two tests.
        >
        > —
        > Reply to this email directly or view it on GitHub
        > <https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-155047954>
        > .
        >

        Show
        githubbot ASF GitHub Bot added a comment - Github user kushal256 commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-155581913 Thanks for your comments. Yes, there should be similar logic for writing graph. Also, I agree, there should be tests, but I'm not sure how to do so when using reflection (Class.forName() and newInstance() for creating new Io.Builder), with mockito's stubs. Any recommendations for how to do this? On Nov 9, 2015 4:13 AM, "Daniel Kuppitz" <notifications@github.com> wrote: > I agree, I'd also like to see that covered by one or two tests. > > — > Reply to this email directly or view it on GitHub > < https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-155047954 > > . >
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user dkuppitz commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-155047954

        I agree, I'd also like to see that covered by one or two tests.

        Show
        githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-155047954 I agree, I'd also like to see that covered by one or two tests.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user spmallette commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-155047685

        Thanks for working on this. I see you did something for reading a graph, but what about writing?

        https://github.com/kushal256/incubator-tinkerpop/blob/master/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java#L296

        Note that the docs needed to be updated:

        https://github.com/apache/incubator-tinkerpop/blob/master/docs/src/implementations.asciidoc#configuration

        Specifically, the configuration description for `gremlin.tinkergraph.graphFormat` needs something to denote this new feature.

        Would be nice to see a test case as well. Perhaps you could stub a reader/writer and assert that the class gets created and the appropriate methods are called. wdyt?

        I'm not sure how others feel, but I think we will wait to merge this until after release of 3.1.0-incubating (in code freeze for that now).

        Show
        githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-155047685 Thanks for working on this. I see you did something for reading a graph, but what about writing? https://github.com/kushal256/incubator-tinkerpop/blob/master/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java#L296 Note that the docs needed to be updated: https://github.com/apache/incubator-tinkerpop/blob/master/docs/src/implementations.asciidoc#configuration Specifically, the configuration description for `gremlin.tinkergraph.graphFormat` needs something to denote this new feature. Would be nice to see a test case as well. Perhaps you could stub a reader/writer and assert that the class gets created and the appropriate methods are called. wdyt? I'm not sure how others feel, but I think we will wait to merge this until after release of 3.1.0-incubating (in code freeze for that now).
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user dkuppitz commented on a diff in the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/141#discussion_r44267313

        — Diff: docs/src/implementations.asciidoc —
        @@ -630,7 +630,8 @@ TinkerGraph has several settings that can be provided on creation via `Configura
        value is specified here, the the `gremlin.tinkergraph.graphFormat` should also be specified. If this value is not
        included (default), then the graph will stay in-memory and not be loaded/persisted to disk.

        gremlin.tinkergraph.graphFormat The format to use to serialize the graph which may be one of the following:
        -`graphml`, `graphson`, or `gryo`. If a value is specified here, the the `gremlin.tinkergraph.graphLocation` should
        +`graphml`, `graphson`, gryo`, or a fully qualified class name that implements Io.Builder interface.
        +If a value is specified here, the the `gremlin.tinkergraph.graphLocation` should
        — End diff –

        Typo."the the" => "then the"

        Show
        githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on a diff in the pull request: https://github.com/apache/incubator-tinkerpop/pull/141#discussion_r44267313 — Diff: docs/src/implementations.asciidoc — @@ -630,7 +630,8 @@ TinkerGraph has several settings that can be provided on creation via `Configura value is specified here, the the `gremlin.tinkergraph.graphFormat` should also be specified. If this value is not included (default), then the graph will stay in-memory and not be loaded/persisted to disk. gremlin.tinkergraph.graphFormat The format to use to serialize the graph which may be one of the following: -`graphml`, `graphson`, or `gryo`. If a value is specified here, the the `gremlin.tinkergraph.graphLocation` should +`graphml`, `graphson`, gryo`, or a fully qualified class name that implements Io.Builder interface. +If a value is specified here, the the `gremlin.tinkergraph.graphLocation` should — End diff – Typo."the the" => "then the"
        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user kushal256 opened a pull request:

        https://github.com/apache/incubator-tinkerpop/pull/141

        TINKERPOP3-886

        Let me know if exceptions should be managed any differently, or if you see any issues.

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

        $ git pull https://github.com/kushal256/incubator-tinkerpop master

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

        https://github.com/apache/incubator-tinkerpop/pull/141.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 #141


        commit 5c4e9660d7fdf3c6d3aa71dd3742a4f0d549cf01
        Author: Kushal <kushal@foodinhood.com>
        Date: 2015-11-08T00:44:43Z

        TINKERPOP3-886: Allow any GraphReader/Writer to be persistence engine for TinkerGraph

        commit 16faa852e350b4d06a1f600078b6604b568438a7
        Author: Kushal <kushal@foodinhood.com>
        Date: 2015-11-08T01:02:08Z

        Merge remote-tracking branch 'upstream/master'


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user kushal256 opened a pull request: https://github.com/apache/incubator-tinkerpop/pull/141 TINKERPOP3-886 Let me know if exceptions should be managed any differently, or if you see any issues. You can merge this pull request into a Git repository by running: $ git pull https://github.com/kushal256/incubator-tinkerpop master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-tinkerpop/pull/141.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 #141 commit 5c4e9660d7fdf3c6d3aa71dd3742a4f0d549cf01 Author: Kushal <kushal@foodinhood.com> Date: 2015-11-08T00:44:43Z TINKERPOP3-886 : Allow any GraphReader/Writer to be persistence engine for TinkerGraph commit 16faa852e350b4d06a1f600078b6604b568438a7 Author: Kushal <kushal@foodinhood.com> Date: 2015-11-08T01:02:08Z Merge remote-tracking branch 'upstream/master'

          People

          • Assignee:
            spmallette stephen mallette
            Reporter:
            spmallette stephen mallette
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development