SteelBridgeLabs / neo4j-gremlin-bolt

Apache License 2.0
0 stars 1 forks source link

Adding your repo to awesome-tinkerpop #38

Closed mohataher closed 7 years ago

mohataher commented 7 years ago

Hello, would you like to get your repo to tinkerpop curated list awesome-tinkerpop?

Please add an adequate short description to your repo and let me know here. I would highly appreciate if you send a pull request.

rjbaucells commented 7 years ago

Yes, sure. I will do it next week. I am out of town...

spmallette commented 7 years ago

On a related note, we could also add this to the the TinkerPop home page in the provider index. It looks like you meet our guidelines for doing so:

http://tinkerpop.apache.org/policy.html

Would you like to see it added?

rjbaucells commented 7 years ago

Sure, I was going to find out the process to get the library listed on the apache Tinkerpop project.. Your help will be very welcome if you can do it..

Thanks

spmallette commented 7 years ago

I've started a thread on the dev list recommending that we add it:

https://lists.apache.org/thread.html/366b3e8fb5ec593d9e9a0aee133ea1d37668eba2a019ed16d2349cdb@%3Cdev.tinkerpop.apache.org%3E

i will add it once that's settled.

mohataher commented 7 years ago

I just added it to awesome-tinkerpop. Check it here.

rjbaucells commented 7 years ago

Thanks

spmallette commented 7 years ago

Note that the provider listing on the TinkerPop home page is also updated. I think that a lot of people have been looking for this functionality. This is a very nice contribution to the TinkerPop and Neo4j communities.

@rjbaucells could you offer any indication as to how well gremlin performs over Bolt? have you made any comparison to the embedded version we have at TinkerPop?

rjbaucells commented 7 years ago

Thanks.

I have not done a comparison with the embedded version since the application I am using the library for uses a remote server. The library performs well and I have not seen any issues so far related to performance. It is good to notice that the BOLT protocol support for Java data types is limited, making the gremlin test suites fail in some cases (BOLT does not support Integer, only Long; casts within the test suite to Integer will fail). see https://neo4j.com/docs/developer-manual/current/drivers/#driver-types for supported data types.

spmallette commented 7 years ago

Hmm - you set the Features properly to not support Integer:

https://github.com/SteelBridgeLabs/neo4j-gremlin-bolt/blob/master/src/main/java/com/steelbridgelabs/oss/neo4j/structure/Neo4JGraphFeatures.java#L219

Do you have an example of such a failing test that you could point me at?

rjbaucells commented 7 years ago

Yes, the features are set with the actual support. I will run the tests again and put some examples, there are explicit casts in the test suite that make the tests fail.

rjbaucells commented 7 years ago

This is one of the examples while running:

@Test
@FeatureRequirementSet(FeatureRequirementSet.Package.SIMPLE)
public void shouldGenerateSameGraph() throws Exception {
}

gremlin-test-3.2.3-sources.jar!/org/apache/tinkerpop/gremlin/algorithm/generator/CommunityGeneratorTest.java line 116

java.lang.ClassCastException: java.lang.Long cannot be cast to java.lang.Integer at org.apache.tinkerpop.gremlin.algorithm.generator.CommunityGeneratorTest$DifferentDistributionsTest.lambda$verticesByOid$4(CommunityGeneratorTest.java:153) at java.util.TimSort.countRunAndMakeAscending(TimSort.java:355) at java.util.TimSort.sort(TimSort.java:234) at java.util.Arrays.sort(Arrays.java:1512) at java.util.ArrayList.sort(ArrayList.java:1454) at java.util.Collections.sort(Collections.java:175) at org.apache.tinkerpop.gremlin.algorithm.generator.CommunityGeneratorTest$DifferentDistributionsTest.verticesByOid(CommunityGeneratorTest.java:152) at org.apache.tinkerpop.gremlin.algorithm.generator.CommunityGeneratorTest$DifferentDistributionsTest.communityGeneratorTest(CommunityGeneratorTest.java:170) at org.apache.tinkerpop.gremlin.algorithm.generator.CommunityGeneratorTest$DifferentDistributionsTest.shouldGenerateSameGraph(CommunityGeneratorTest.java:121) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26) at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27) at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:55) at org.junit.rules.RunRules.evaluate(RunRules.java:20) at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) at org.junit.runners.ParentRunner.run(ParentRunner.java:363) at org.junit.runners.Suite.runChild(Suite.java:128) at org.junit.runners.Suite.runChild(Suite.java:27) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) at org.junit.runners.ParentRunner.run(ParentRunner.java:363) at org.junit.runners.Suite.runChild(Suite.java:128) at org.junit.runners.Suite.runChild(Suite.java:27) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) at org.junit.runners.ParentRunner.run(ParentRunner.java:363) at org.junit.runners.Suite.runChild(Suite.java:128) at org.apache.tinkerpop.gremlin.AbstractGremlinSuite.runChild(AbstractGremlinSuite.java:212) at org.apache.tinkerpop.gremlin.AbstractGremlinSuite.runChild(AbstractGremlinSuite.java:50) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) at org.junit.runners.ParentRunner.run(ParentRunner.java:363) at org.junit.runner.JUnitCore.run(JUnitCore.java:137) at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:78) at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:212) at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:68) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at com.intellij.rt.execution.application.AppMain.main(AppMain.java:140)

spmallette commented 7 years ago

i see - that's an example of a test with a bad FeatureRequirement annotation (or a bad implementation depending on how you look at it). The test could either have a String for the "oid" value or the test should have a FeatureRequirement for FEATURE_INTEGER_VALUES.

would you like to submit a pull request to correct that (and others like it)? That way your test suite will pass in more legit fashion for next release. if you aren't so inclined to do a pull request, i can get it fixed (and others if you tell me what they are). wdyt?

rjbaucells commented 7 years ago

@spmallette I do not have time now to create a pull request, I can do it in about a month or so... In general I think it would be better to be able to specify a default data type for all tests that are not type-specific in the test suites, a value provider or something that let the implementor choose a type that is fully supported by the implementation. I know that Integer should be common enough, but in this case is not supported...

As a starting point, all tests defined in org.apache.tinkerpop.gremlin.algorithm.generator.CommunityGeneratorTest.java fail due to the explicit Integer cast verticesByOid() (line 153). The same pattern is implemented in other parts of the test suite (org.apache.tinkerpop.gremlin.algorithm.generator.DistributionGeneratorTest.java, line 145)

The ProcessStandardSuite.class is in better shape, it is following the Feature definition but too many tests are skipped due to dependency on non-supported types in the BOLT protocol.

spmallette commented 7 years ago

I do not have time now to create a pull request, I can do it in about a month or so.

ok - not a problem - i will fix the "generator" tests myself then.

I know that Integer should be common enough, but in this case is not supported...

we've never seen a problem with such basic data types like Integer. this seems like a problem specific to Bolt. making the tests more type agnostic would be a fat undertaking i think.

rjbaucells commented 7 years ago

@spmallette I forgot I had removed the datatype verification at the time of setting a property value to a Vertex/Edge. After I put it back again the org.apache.tinkerpop.gremlin.algorithm.generator.CommunityGeneratorTest tests fail with the exception (java.lang.IllegalArgumentException: Property value [0] is of type class java.lang.Integer is not supported). The errors are thrown when trying to assign an Integer value to a vertex property (afterLoadGraphWith(), line 146)

spmallette commented 7 years ago

yeah - i guess we never realized an int was being used in that test. it should have had:

@FeatureRequirement(featureClass = Graph.Features.VertexPropertyFeatures.class, feature = Graph.Features.VertexPropertyFeatures.FEATURE_INTEGER_VALUES)

i'm adding that now to those tests, so it should be fixed in the future for you.

rjbaucells commented 7 years ago

Same case on:

org.apache.tinkerpop.gremlin.algorithm.generator.DistributionGeneratorTest, afterLoadGraphWith() line 181

org.apache.tinkerpop.gremlin.structure.EdgeTest, shouldNotHaveAConcurrentModificationExceptionWhenIteratingAndRemovingAddingEdges() line 283, 286

Thanks,

RJB

spmallette commented 7 years ago

ok - fixed those. should be available on 3.1.6 and forward.

rjbaucells commented 7 years ago

Thanks, I will try it as soon as it is available

rjbaucells commented 7 years ago

Hi @spmallette,

I modified the travis build script to execute the Tinkerpop integration tests to validate the changes you did on the up-coming 3.1.6 release but I am still getting datatype related errors while running the tests. You can see the details on the build log:

https://travis-ci.org/SteelBridgeLabs/neo4j-gremlin-bolt#L1193

https://travis-ci.org/SteelBridgeLabs/neo4j-gremlin-bolt#L1261

https://travis-ci.org/SteelBridgeLabs/neo4j-gremlin-bolt#L1316

spmallette commented 7 years ago

not sure if it matters but those links you provided to the build logs didn't seem to point to much when i clicked on them. in any case, i did scroll through and find the errors. it didn't appear as if i'd published a fresh snapshot after i made my changes to the code on 10/26. i just did that now so hopefully if you restart your travis job, you'll get a good build at this point.

https://repository.apache.org/content/groups/snapshots/org/apache/tinkerpop/gremlin-test/3.1.6-SNAPSHOT/

rjbaucells commented 7 years ago

Thanks @spmallette, I ran the test suite again and I only have one error now:

org.apache.tinkerpop.gremlin.structure.PropertyTest.shouldAllowRemovalFromEdgeWhenAlreadyRemoved() (line 108) is adding an Integer value without testing support for it...

Is there any plans for a release with these changes?

Current tinkerpop version is 3.2.3, are the test fixes going to be applied to a version after it (they are only on 3.1.6-SNAPSHOT)?

Thanks for your help...

spmallette commented 7 years ago

dah - we're missing a Integer feature definition on shouldAllowRemovalFromEdgeWhenAlreadyRemoved. I can add that and get a new SNAPSHOT published. Both of my systems are busy in long builds right now, but as soon as one is free i'll do that and post back here.

Is there any plans for a release with these changes?

There hasn't been any discussion in the community about release plans at this point. We do have a good body of changes in place right now, so perhaps it's time to do one.

Current tinkerpop version is 3.2.3, are the test fixes going to be applied to a version after it (they are only on 3.1.6-SNAPSHOT)?

The fixes have been made on the 3.1.x line but they are merged forward to 3.2.x and 3.3.x, So, they should be on 3.16, 3.2.4 and 3.3.0 when those releases are made.

spmallette commented 7 years ago

just deployed a fresh SNAPSHOT of 3.1.6. hopefully that solves all your issues now. i also deployed 3.2.4-SNAPSHOT and 3.3.0-SNAPSHOT, in case you want to start testing against those. note that we likely won't get to an official release of 3.3.0 for a while - it's still in fairly early stages. the community expects to do a few milestone releases on that line of code as it will likely contain breaking changes to contend with and for folks to get used to. if you don't already follow the dev mailing list, you might want to consider subscribing:

https://lists.apache.org/list.html?dev@tinkerpop.apache.org

all technical discussion as well as release related talk happen there. as a "provider" you might find the discussions we have there useful/interesting.

rjbaucells commented 7 years ago

Good news, the test suite passed without errors:

Tests run: 1627, Failures: 0, Errors: 0, Skipped: 1039

I test it using 3.2.4-SNAPSHOT. As you can see there are a some tests that are been skipped due to data type support missing in the BOLT protocol....

Thanks @spmallette for your help... I will be waiting on a release on your side to do the same here.

RJB

spmallette commented 7 years ago

I'm not sure if this is useful to you, but I just implemented this for 3.2.4 (still under review as a pull request):

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

If there are expensive resources that could be released as a result of the way that bolt works, this could be a way for you to allow users to release those resources.

rjbaucells commented 7 years ago

Yes, that will help once OLAP API is implemented on this project. So far we have implemented the OLTP gremlin API... (see #46 for details)

rjbaucells commented 7 years ago

Closing this issue after testing the library using the Apache Tinkerpop 3.2.4 release