You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by edi-bice <gi...@git.apache.org> on 2015/06/19 22:43:19 UTC
[GitHub] incubator-tinkerpop pull request: hadoop-gremlin compatible GraphS...
GitHub user edi-bice opened a pull request:
https://github.com/apache/incubator-tinkerpop/pull/85
hadoop-gremlin compatible GraphSONLegacyReader based on LegacyGraphSONReader and GraphSONReader
I have been trying - as the group discussion shows - to load a huge Titan 0.4.4 graph exported to GraphSON into Titan 0.9M2:
https://groups.google.com/d/msg/aureliusgraphs/7yvrtPrf_9w/WPQ4ktaXJdQJ
Haven't tested the changes in this pull request so I don't expect it to be merged. Yet. Please review and make suggestions.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/edi-bice/incubator-tinkerpop master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-tinkerpop/pull/85.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 #85
----
commit 32e8ff6da32969d23863ab64ebcf7fe2b30ca0ed
Author: ebice <eb...@behaviormatrix.com>
Date: 2015-06-19T12:53:59Z
Starting to create a GraphSONReader capable of reading TP2 GraphSON and usable via hadoop-gremlin
commit 0fa53ea570b8b52e54f4bec618313f34fa1cfbda
Author: ebice <eb...@behaviormatrix.com>
Date: 2015-06-19T15:47:33Z
Merge branch 'master' of https://github.com/apache/incubator-tinkerpop
Conflicts:
gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/io/IoTest.java
commit 536946e36afd84dbc5c921bb74ef8010c5822778
Author: ebice <eb...@behaviormatrix.com>
Date: 2015-06-19T20:35:38Z
Merge branch 'master' of https://github.com/apache/incubator-tinkerpop
Conflicts:
gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/io/IoTest.java
Continued fleshing out GraphSONLegacyReader for use with hadoop-gremlin
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
Re: [GitHub] incubator-tinkerpop pull request: hadoop-gremlin compatible GraphS...
Posted by Marko Rodriguez <ok...@gmail.com>.
Hello,
I would prefer to keep internal StarGraph methods private. Can the code simply not use addEdge() which is public?
Marko.
On Jun 22, 2015, at 9:01 AM, spmallette <gi...@git.apache.org> wrote:
> Github user spmallette commented on the pull request:
>
> https://github.com/apache/incubator-tinkerpop/pull/85#issuecomment-114141984
>
> Thanks for attempting to expand this capability.
>
> You added `GraphSONLegacyReader` which basically reads TP2 GraphSON in the titan adjacency list style. Is that the primary difference between it and the original `LegacyGraphSONReader`? If so, i wonder if they couldn't be combined in some way such that you just could configure in the `Builder` whether to use adjacency list or edge list style...wdyt?
>
> I see you had to make these `public` on `StarGraph`:
>
> https://github.com/apache/incubator-tinkerpop/pull/85/files#diff-60db5b3a8acf1e835406ee1f75f02903R289
> https://github.com/apache/incubator-tinkerpop/pull/85/files#diff-60db5b3a8acf1e835406ee1f75f02903R305
>
> I've pulled back from doing that a few times now to try to reduce exposure of too much of the internals of `StarGraph`.
>
> @okram do you have any thoughts on that?
>
> i gave the record reader/input format portion of this quick review, but don't have anything specific to say on that just yet - @dkuppitz any thoughts there?
>
>
>
> ---
> 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 infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
[GitHub] incubator-tinkerpop pull request: hadoop-gremlin compatible GraphS...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/85#issuecomment-114141984
Thanks for attempting to expand this capability.
You added `GraphSONLegacyReader` which basically reads TP2 GraphSON in the titan adjacency list style. Is that the primary difference between it and the original `LegacyGraphSONReader`? If so, i wonder if they couldn't be combined in some way such that you just could configure in the `Builder` whether to use adjacency list or edge list style...wdyt?
I see you had to make these `public` on `StarGraph`:
https://github.com/apache/incubator-tinkerpop/pull/85/files#diff-60db5b3a8acf1e835406ee1f75f02903R289
https://github.com/apache/incubator-tinkerpop/pull/85/files#diff-60db5b3a8acf1e835406ee1f75f02903R305
I've pulled back from doing that a few times now to try to reduce exposure of too much of the internals of `StarGraph`.
@okram do you have any thoughts on that?
i gave the record reader/input format portion of this quick review, but don't have anything specific to say on that just yet - @dkuppitz any thoughts there?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: hadoop-gremlin compatible GraphS...
Posted by edi-bice <gi...@git.apache.org>.
Github user edi-bice commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/85#issuecomment-114262218
Sounds good @spmallette. Will rework along those lines. First order of business was to get the project set up properly on Intellij with tests running OK so I can test what I add. Ran into this problem and wanted to thank you for the solution.
https://groups.google.com/forum/#!searchin/gremlin-users/intellij/gremlin-users/PapM7Y94T68/xqsh_fIwkDoJ
Now I have tests passing and am good to go.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: hadoop-gremlin compatible GraphS...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/85#issuecomment-130824115
Thanks for the update. I'll start to play with what you have a little bit. Merged your stuff to this branch:
https://github.com/apache/incubator-tinkerpop/tree/tp30-legson
It's not introducing breaking API changes that I can tell so I'm hoping it can slip into 3.0.1 unless I see some unexpected risk somewhere along the line.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: hadoop-gremlin compatible GraphS...
Posted by edi-bice <gi...@git.apache.org>.
Github user edi-bice commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/85#issuecomment-114586021
I needed a test resource in the legacy GraphSON adjacency list format and went with the following http://s3.thinkaurelius.com/docs/titan/0.5.4/hadoop-getting-started.html#_graph_of_the_gods_with_hadoop
Is it OK to (re)introduce this data set or should I work on converting one of the others, say Grateful Dead, into the legacy GraphSON adjacency list format?
Another thing is there's an implicit expectation in the code that each of the graph resources is available in all the supported formats - Kryo, Json, XML. I've tried to add conditionals in the right places but will see if I can do better as I go along.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: hadoop-gremlin compatible GraphS...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/85#issuecomment-114598156
My vote would be to convert the TinkerPop graphs to legacy format - sorry - i know that's a hassle, but I think we want to stay consistent. I don't think you need to do them all - just the classic graph and possibly grateful dead (if you feel like it). That should be easy actually as they already exist in TP2.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: hadoop-gremlin compatible GraphS...
Posted by edi-bice <gi...@git.apache.org>.
Github user edi-bice commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/85#issuecomment-131101902
Yes, you're right, there's a lot of duplication in the utilities. Was rushing trying to get the data converted as I was heading out the door. Not sure about your 3d point. I put in a lot of effort so it could parse and convert all of the TP example datasets. Also not sure where it belongs really. Maybe a separate utility.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: hadoop-gremlin compatible GraphS...
Posted by mhfrantz <gi...@git.apache.org>.
Github user mhfrantz commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/85#issuecomment-131114467
What about starting a "tinkerpop-migration" package? It could be a separate GitHub repo, since full migration from TP2 is not supported (I think).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: hadoop-gremlin compatible GraphS...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/85#issuecomment-114681100
formerly known as `graph-example-2.xml`:
https://github.com/tinkerpop/gremlin/blob/master/data/graph-example-2.xml
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: hadoop-gremlin compatible GraphS...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-tinkerpop/pull/85
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: hadoop-gremlin compatible GraphS...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/85#issuecomment-130691344
@edi-bice anything new with this PR? no rush, was just wondering how things were going
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: hadoop-gremlin compatible GraphS...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/85#issuecomment-114177314
> yes that is the primary difference...If you think it makes more sense to combine and use the Builder I can do that.
I guess a design might be to have one `LegacyGraphSONReader` but then have two separate "internal" implementations of it (i.e. one for adjacency list and one for edge list). Based on the `Builder` options you just choose the right internal class to use in the `LegacyGraphSONReader`. That way things stay separate but all notion of "legacy" to end-users is hidden behind a single `LegacyGraphSONReader` class.
> Just wanted to get your opinion and make sure I was going in the right direction.
I think it's good that you're taking an iterative approach to this and requesting feedback early. We'll keep an eye out for your next revision of this.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: hadoop-gremlin compatible GraphS...
Posted by edi-bice <gi...@git.apache.org>.
Github user edi-bice commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/85#issuecomment-130784640
@spmallette Changed jobs and not using Titan/TP yet. Might be a while and then won't need to import any legacy graphs. Would love to finish it off though - it seemed pretty close to me. Believe it was all functional but not yet wrapped nicely in a single class using Builder to call out the two separate internal implementations.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: hadoop-gremlin compatible GraphS...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/85#issuecomment-131079499
I'm not so sure that this is as close to being done as you thought. To name a few issues I noticed besides what you mentioned:
* There's a lot of duplicate code to consolidate around the different "utilities"
* We'd need to remove the Jettison dependency
* Ultimately "graph of the gods" data set would need to be replaced by one or more of the common TinkerPop datasets.
I'm also wondering how useful this is now outside of Titan/Faunus users. The id system forces longs. It uses `COMPACT` GraphSON which is "non-standard" in a sense (again for Titan). That doesn't match the original reader we supplied which uses `EXTENDED`.
This almost feels more like a PR for Titan as it sorta bound to that implementation. Short of users generating their own files like this, I'm not sure that i see the value to TinkerPop. Any thoughts on the matter?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: hadoop-gremlin compatible GraphS...
Posted by edi-bice <gi...@git.apache.org>.
Github user edi-bice commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/85#issuecomment-114166588
@spmallette yes that is the primary difference. I wasn't even aware of the two types of GraphSON until a couple of days ago. It's been a bit confusing dealing with GraphSON in its two flavors and then all the changes between TP2 and TP3. If you think it makes more sense to combine and use the Builder I can do that.
Yes, I couldn't figure out how to add the StarGraph edges without making those methods public. Also extracted out LegacyGraphSONUtility hoping to use its vertex/edgeFromJson but then went with StarGraph. Will see what else of it I am still using and try to bring it back as inner class of the LegacyGraphSONReader I believe.
I apologize for some of the mess. Just wanted to get your opinion and make sure I was going in the right direction. Thanks for the feedback.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: hadoop-gremlin compatible GraphS...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/85#issuecomment-114263012
hmmm - i will update the README with that snippet (which i'd forgotten about) - it has a section for IDE setup.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: hadoop-gremlin compatible GraphS...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/85#issuecomment-131113849
I just meant that I don't think we should have the "graph of the gods" in the TinkerPop datasets, so if that was being used somewhere (i didn't dig that deep) then it should be replaced.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: hadoop-gremlin compatible GraphS...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/85#issuecomment-135477510
I think that conceptually this is a nice idea but the more I look at it the more I think this should live in it's own GitHub repo. Thanks for giving this a try, but I don't think we can merge this.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---