Skip to content

Update to Tinkerpop 3.2.3 and fix tests#1

Merged
dylanht merged 2 commits intodylanht:upgrade-to-tinkerpop-3.2.0-incubatingfrom
ngageoint:tinkerpop-3.2.3
Dec 1, 2016
Merged

Update to Tinkerpop 3.2.3 and fix tests#1
dylanht merged 2 commits intodylanht:upgrade-to-tinkerpop-3.2.0-incubatingfrom
ngageoint:tinkerpop-3.2.3

Conversation

@sjudeng
Copy link

@sjudeng sjudeng commented Nov 9, 2016

After updating to Tinkerpop 3.2.3 I found there were a large number of test errors/failures, mostly involving OLAP traversals, across all Titan indexing backends when running TinkerPop tests (mvn clean install -fn -Dtest.skip.tp=false -DskipTests=true). The PR includes updates that resolve these issues, with some notes as described below. With these updates all tests, including both default tests and TinkerPop tests (e.g. mvn clean install -Dtest.skip.tp=false), are passing.

  • Several tests depend on this TinkerPop PR
  • Explicitly attaching ReferenceElements before persisting in FulgoraGraphComputer to resolve numerous tests failures associated with missing properties
  • Opting out of GraphComputerTest#shouldSupportGraphFilter since FulgoraGraphComputer doesn't support graph filters but also can't throw the expected exception without breaking a number of tests that otherwise pass
  • TraverserSet and HashMap can now be serialized, but this uses Java serialization so performance will be very poor. Note this update, in particular the support for TraverserSet serialization, might be relevant to the issue with storing halted traversals mentioned in your PR.
  • Titan assigned vertex ids can vary significantly between runs and ids for subsequently added vertices can often be smaller than those for existing vertices. This caused sporadic issues in two tests as described below. I think these issues should be addressed on the TinkerPop side but doing so will require more work to come up with appropriate test cases (not to mention solutions). In the meantime these issues were resolved by adding appropriate configuration to test Titan GraphComputerProvider implementations (e.g. InMemoryGraphComputerProvider, etc.) to ensure vertex ids are evenly spaced and increasing in tests.
    • PeerPressureTest#g_V_peerPressure_byXclusterX_byXoutEXknowsXX_pageRankX1X_byXrankX_byXoutEXknowsXX_timesX2X_group_byXclusterX_byXrank_sumX - The vertex program appears to cluster by vertex id. I don't think this will work (consistently) for Titan unless clustering by some other property.
    • UnionTest#g_VX1_2X_localXunionXoutE_count__inE_count__outE_weight_sumXX - When the assigned id of the first vertex in the test, marko, is less than that of the second vertex, vadas, then the test passes. Otherwise the test fails. In this case the local traversal is executed twice on the second (smaller id) vertex. The issue can be resolved by cloning the local step in WorkerExecutor if the associated local traversal starts with a UnionStep ... but that's just a hack and can't be recommended to TinkerPop (especially not without a test case).
  • The PR also includes an extra commit for formatting and code cleanup. One note on this is that I noticed you moved the TitanGraphComputer interface into the FulgoraGraphComputer implementation. Was this intended? If not necessary I think it makes the overall PR cleaner if that refactoring is not included.

…significant updates to FulgoraGraphComputer and associated memory implementation, support for GraphSON 2.0 and support for interrupts in HBase backend. Opt out of IoTest#shouldReadGraphMLWithNoEdgeLabel and GraphComputerTest#shouldSupportGraphFilter (see reasons in OptOut declarations). Skip titan-hadoop-1 tests (hadoop1 is no longer supported).
@dylanht dylanht merged commit f88f7cc into dylanht:upgrade-to-tinkerpop-3.2.0-incubating Dec 1, 2016
@dylanht
Copy link
Owner

dylanht commented Dec 1, 2016

@sjudeng sorry for taking so long to get to this - thanks so much for your PR! This certainly cleans up the work I did and then some. Merged, and will likely be pushing some changes to this repo soon as well.

@sjudeng
Copy link
Author

sjudeng commented Dec 1, 2016

Great, thanks for getting this merged. You might also consider updating the title in your PR to say 3.2.3 instead of 3.2.0 incubating.

@pluradj
Copy link

pluradj commented Dec 2, 2016

I just gave this a try, and the Gremlin Server seems to have trouble coming up.

$ ./bin/titan.sh start
Forking Cassandra...
Running `nodetool statusthrift`.. OK (returned exit status 0 and printed string "running").
Forking Elasticsearch...
Connecting to Elasticsearch (127.0.0.1:9300).. OK (connected to 127.0.0.1:9300).
Forking Gremlin-Server...
Connecting to Gremlin-Server (127.0.0.1:8182)............................. timeout exceeded (60 seconds): could not connect to 127.0.0.1:8182

Interestingly, if I remove the nashorn script engine from the gremlin-server.yaml it starts up just fine. I confirmed this wasn't an issue with the previous SHA.

@sjudeng
Copy link
Author

sjudeng commented Dec 5, 2016

Thanks @pluradj. Sounds like a classpath issue. I'll take a look.

@sjudeng
Copy link
Author

sjudeng commented Dec 8, 2016

I just submitted a new PR with the update to remove the nashorn ScriptEngine reference. I went back to core gremlin-server and found this commit where it appears support for pure nashorn ScriptEngine was removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants