Support Apache TinkerPop 3.2 about janusgraph HOT 12 CLOSED

@dylanht The branch below updates https://github.com/dylanht/titan/tree/upgrade-to-tinkerpop-3.2.0-incubating to retarget to JanusGraph.

https://github.com/ngageoint/janusgraph/tree/tinkerpop-upgrade

The full test suite including tinkerpop tests (e.g. mvn clean install -Dtest.skip.tp=false) has been running for about 36 hours and is about 75% complete with no errors so far. I'll add a comment here when complete.

from janusgraph.

The tests finished. There were some last minute (literally) failures in janusgraph-solr, but I don't think they were real errors as they only occurred under one account and not another and I couldn't reproduce on other systems.

As before several tests require apache/tinkerpop#458, which was merged after the TikerPop 3.2.3 release. Also the tests were run on an integrated master branch, which included this as well as some other feature branches.

I'm waiting for a legal review by my organization before I can submit PRs (e.g. sign CLA). I'll submit when that goes through or you can when you get around to it.

The TinkerPop 3.3.x work looks interesting. It took a ton of debugging to update FulgoraGraphComputer to TinkerPop 3.2.x. I would have loved to ditch it. But this is the computer used to test all TinkerPop process/OLAP test suites against JanusGraph. I think the only way to get rid of it for little cost would be to reuse TinkerPop computer/actors for tests. I looked into this briefly but the only possible option I saw was SparkGraphComputer, which seemed like a non-starter from a dependency-hell standpoint if nothing else. Maybe I overlooked something here or TinkerPop 3.3.x will add another option.

Otherwise it might make sense to write a custom implementation of AbstractHadoopGraphComputer purely for in-memory JanusGraph testing. This might be easier than to update the existing implementation to support TinkerPop 3.3.x actors/etc.. I think at least some of the complexity of FulgoraGraphComputer is to make it perform well, since it was originally intended to be used beyond testing, which might not be necessary going forward.

from janusgraph.

@sjudeng fantastic - I was about self-assign, but it looks like you're about ready to submit a PR. If that's your intention I say go for it - I have something pressing to focus on until Thursday, but I'll checkout the branch you have going and have a closer look then even if there's no PR yet. I had a quick look and I think this is great for two reasons:

1. Inaugural step towards modernizing the functionality we provide via TinkerPop.
2. Give everyone who reviews the PR a nice glimpse into JanusGraph's internals which will probably yield plenty of ideas for future work, optimizations, and general improvements.

Thanks a lot for doing this - it looks like with what TinkerPop 3.3.x is going for with this and this we may be in a position to rethink and simplify quite a few things including getting rid of the on-board single-machine in-memory GraphComputer that was such a pain to update to 3.2.x completely. Would be interested to know your thoughts on that.

from janusgraph.

Apache TinkerPop 3.2.4 will be released soon, so we should be able to pick up the required fix mentioned above as well as a critical security fix for Groovy. Are there any other TinkerPop fixes we need @sjudeng @dylanht ?

from janusgraph.

No all 40 hours worth of TinkerPop tests pass with the above fix.

from janusgraph.

TinkerPop 3.2.4 was released yesterday (thanks @pluradj). After updating there are a number of test failures/errors. All issues appear to be in org.apache.tinkerpop.gremlin.process.traversal.step.map.ProfileTest. Hopefully these won't be too hard to squash but in the meantime #78, which is currently under review, offers a stable update to the previous release (3.2.3).

from janusgraph.

from janusgraph.

@pluradj I squashed my commits in the branch that @sjudeng merged in for PR #78 and "remembered" that I was running into issues with profiling in the tests back in May this year when I tried to update support for TinkerPop to 3.2.x - I noted in the commit message a few classes in JanusGraph that merit looking into re: profiling, and I believe the changes that https://github.com/rjbriody made to profiling in TinkerPop in these PRs could help steer us on this one, primarily:
apache/tinkerpop#273

But also:
apache/tinkerpop#242
apache/tinkerpop#195
apache/tinkerpop#193

And it looks like this could be the "starting point" for how JanusGraph expects profiling to work:
apache/tinkerpop#58

I think it may be in order to poke around and check that the stable update is in fact stable with respect to profiling - hopefully it's simple enough to rectify that we don't need to post a separate issue.

from janusgraph.

@dylanht Thanks for doing the squash work. I'll get that PR updated. The TinkerPop profile tests do run without issue in #78 based on version 3.2.3 but maybe the tests added in 3.2.4 show issues hidden by previous gaps in testing is what you're thinking? Are you okay with moving forward with the #78 merge and then working these issues as part of follow-on work to update to 3.2.4?

from janusgraph.

@sjudeng No problem sorry for the delay on my end - yes, I am okay with moving forward on #78 at this point. Reading over my commits and seeing this here gave me a bit of a pause, but since the tests run without issue I don't think paranoia over what they could be missing between 3.2.3 and 3.2.4 is helpful. If I laid any traps for us in my commits that you tidied up for #78 (thanks again for that) my concerns are documented here and in the squashed commit message. We can sort it out as we target 3.2.4 as you said.

from janusgraph.

TinkerPop 3.2.4 Released

https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/gremlin-users/qQEF1l8Pm6E/ETU2bD0rCwAJ

from janusgraph.

Closing this since we've updated to 3.2.3 but also created a new issue #173 for updating to TP 3.2.4.

from janusgraph.