[GitHub] tinkerpop issue #838: TINKERPOP-1822: Add Depth First Search repeat step opt...
Github user krlohnes commented on the issue: https://github.com/apache/tinkerpop/pull/838 Thanks @spmallette I'll hold off on any more changes until things are worked out with that DISCUSS thread ---
[GitHub] tinkerpop issue #838: TINKERPOP-1822: Add Depth First Search repeat step opt...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/838 > I'll take a swing at the js and .net stuff and see how that plays out for now. i meant to mention this in my last post but forgotjust a suggestion but you might want to wait a bit for feedback before forging ahead too much further. maybe let that DISCUSS thread you started play its course a bit first. already some questions popping up over there and i know i have some myself. don't want you to have to do more work that is necessary on this in case something has to change. ---
[GitHub] tinkerpop issue #838: TINKERPOP-1822: Add Depth First Search repeat step opt...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/838 Sorry, but I haven't had time to dig into this into any great detail. Hopefully, sometime this week. Here's some quick comments based on the discussion: > I actually think we should make DFS the default for OLTP > I'd want to give users a bit of time to know this exists, and make the switch after there's some awareness of the option to toggle between the two. That's a topic that is probably worth a DISCUSS thread on the dev mailing list. I typically reference threads like that as a link in the associated JIRA. > mvn clean test That should work, but typically only after `mvn clean install` of a new branch. gremlin-shaded and hadoop-gremlin typically cause that problem, as a result, you really can only do `mvn clean install` at least until 3.4.x when i get some changes in (maybe...hopefully). ---
[GitHub] tinkerpop issue #838: TINKERPOP-1822: Add Depth First Search repeat step opt...
Github user krlohnes commented on the issue: https://github.com/apache/tinkerpop/pull/838 @mpollmeier I was running `mvn clean install` locally, and had the same issue with that test stalling out. A few builds before this had different errors in Travis, not just stalling out, so it looks like some of the changes i pushed EOD took care of those. I can take care of the js issue and the .net stuff. I'm not sure about the `TinkerGraphProcessComputerTest` stalling out though or the Travis stuff stalling out. I'll take a swing at the js and .net stuff and see how that plays out for now. ---
[GitHub] tinkerpop issue #838: TINKERPOP-1822: Add Depth First Search repeat step opt...
Github user mpollmeier commented on the issue: https://github.com/apache/tinkerpop/pull/838 If I run `mvn clean test` I also get heaps of compilation errors, but that's the same with master for me. E.g. ``` [ERROR] symbol: class JsonGenerator [ERROR] location: class org.apache.tinkerpop.gremlin.structure.io.graphson.TraversalSerializersV3d0.TraversalJacksonSerializer [ERROR] /home/mp/Projects/tinkerpop/tinkerpop3/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/TraversalSerializersV3d0.java:[74,99] cannot find symbol [ERROR] symbol: class SerializerProvider [ERROR] location: class org.apache.tinkerpop.gremlin.structure.io.graphson.TraversalSerializersV3d0.TraversalJacksonSerializer [ERROR] -> [Help 1] ``` Is that the same you're getting @krlohnes? If you just run `mvn clean install` as per http://tinkerpop.apache.org/docs/current/dev/developer/#building-testing it compiles fine (for me) and also runs tests which all pass, but then it stalls at `TinkerGraphProcessComputerTest`. [Travis](https://travis-ci.org/apache/tinkerpop/builds/366306070) reports four failed builds. Two of them time out, no idea what's going on there. The other two fail for a missing integration into .net and JS. Not really my cup of tea... Note: I also had to comment out `false` in `gremlin-core/pom.xml`. No idea what that is, but it runs into a NPE for me. @robertdale how do you normally run the tests, and how can we fix the .net/JS stuff? ---
[GitHub] tinkerpop issue #838: TINKERPOP-1822: Add Depth First Search repeat step opt...
Github user robertdale commented on the issue: https://github.com/apache/tinkerpop/pull/838 Look at the Serializers in `./gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/` ---
[GitHub] tinkerpop issue #838: TINKERPOP-1822: Add Depth First Search repeat step opt...
Github user krlohnes commented on the issue: https://github.com/apache/tinkerpop/pull/838 I'm definitely still missing something around serialization. All of the traversals that are failing in the gryo side of things work in the console. I'm fairly unfamiliar with that part of the code, but I'll look a bit this weekend if I have some time. If someone more familiar with it sees something obvious I've missed, let me know and I'm happy to add it. ---
[GitHub] tinkerpop issue #838: TINKERPOP-1822: Add Depth First Search repeat step opt...
Github user krlohnes commented on the issue: https://github.com/apache/tinkerpop/pull/838 I figured out the errors that I was getting with serialization. I think I just missed registering `SearchAlgo` in a spots. I have some legitimate test failures now I think, which makes fixing things quite a bit easier, I can take a swing at those over the weekend. @mpollmeier I do like the test you're suggesting, I was trying to come up with a test case for an existing dataset in the test infrastructure to make sure things worked there for now. I think testing on a k-ary tree might be worth while too. A lot of bugs I found when I was testing the general idea were found in that case. As far as making DFS the default, my only concern would be breaking existing traversals. I don't know if there are traversals out there that depend on `repeat` being BFS that would need to be fixed with a change for the default. I'd think there would be a desire to not change that behavior default until the next minor version bump. I'd want to give users a bit of time to know this exists, and make the switch after there's some awareness of the option to toggle between the two. ---
