Remove null values to fix TINKERPOP-1923#818
Conversation
|
Thanks for looking into this issue. I saw your issue in JIRA and your SO question, but just haven't had a chance to really dig into it. I can see how your solution would get rid of the Also, if this is the right fix, could you get rid of the use of Finally, could you please retarget this PR to the tp32 branch as I think this fix should be applied across all currently maintained versions? Note that we have a release coming in the next two weeks so this PR arrived just in time - thanks again. |
jbaker-nca
force-pushed
the
TINKERPOP-1923
branch
from
87e5104 to
75f52cc
Compare
|
Hi @spmallette, I've made the changes requested. My understanding is that in some cases, when removing nodes and/or edges, the collection of edges on a vertex can end up containing null values (I believe they should be properly removed rather than replaced with nulls, but my knowledge of the Tinkerpop codebase is very limited so I wasn't sure why this would be happening). These null values were causing issues when looping across the collection, which was resulting in the exception thrown. |
|
@jbaker-nca Is there a test case that would fail without this fix so it can catch regressions? |
right, i think i followed that part - i was asking more about the "why" nulls were showing up in the first place, but it sounds like you hadn't gotten that far (not a problem - i understand that you're new to the code base). I think we need to sort out the "why" before we merge a "fix" or else that fix might not be the right one. |
|
@spmallette I've been working with @jbaker-nca to understand this issue a bit more. Our use case for discovering this bug was essentially merging two separate graphs into a single graph. When two vertices are to be merged we copy the edges from the original vertex to a new one and then proceed to remove the edges on that vertex using the following: Object[] edgeIdsArr = edgeIds.toArray();
if(edgeIdsArr.length > 0)
graph.traversal().E(edgeIdsArr).drop().iterate();Once the vertices are merged we then remove the original vertices from the graph: Object[] ids = mergeGroup.stream().map(Element::id).filter(Objects::nonNull).toArray();
if(ids != null && ids.length > 0)
graph.traversal().V(ids).drop().iterate();This second traversal on vertex removal is where the error would occur yielding the following exception: I noticed the following Google Groups post - https://groups.google.com/forum/#!topic/gremlin-users/CGkuoBkNMGI - which indicates that when removing a vertex the edges should be removed at the same time. Therefore I modified our code to no longer remove the edges and only remove the vertices. This appears to have fixed our issue and we have reverted back to Tinkerpop 3.3.1 without any further issues. However, I suspect that one should be able to remove edges and then vertices without encountering the issue. |
|
@rcaunt how did you do this part:
|
|
The copying code is as follows (I've simplified a little to make it clearer what's happening, as it's actually part of a bigger chunk code): |
|
I think I'm going to have to close this because I can't recreate the problem with the current information supplied and I don't want to merge this PR as-is because it seems like it's just making the issue go away without actually addressing the root of the issue. As there is a workaround (i.e. drop the vertex which drops the edges with it) I don't think we have a bad blocker here. If the issue arises in the future and can be recreated, I'd be happy to re-open this. I still feel like there's something multi-threaded at the root of this somehow, but I can't think of how since you've confirmed that you don't have anything like that going on. If the issue does stem from multiple threads modifying some aspect of the TinkerGraph at the same time then this problem isn't an issue as TinkerGraph is not meant to be thread-safe for mutations. |
|
The issue is still there and happens very often with the I use gremlin-server as a replacement for AWS Neptune locally and on CI. Main use case - create tens of thousands of vertices and nodes(in parallel from multiple processes), run some queries, drop all the data with The script that imports the data heavily relies on the upsert pattern, multiple processes may try to upsert same vertices and edges in parallel. After the first error, I'm able to drop all vertices one by one except the one which probably caused the error. Attempts to drop it lead to the same error. That vertex was created with an upsert. Server has to be restarted to act normally. From my experience the error occurs more often if I use more parallel processes to import my data. |
|
I prepared a script that 100% reproduces the issue: https://github.com/zhulik/gremlin_server_error_demo |
|
Well, wrapping lines that may cause the issue in a try...catch block fixes it 😆 |
|
@zhulik This appears to be a TinkerGraph problem. If the issue occurs when doing parallel mutations to the graph then that likely expected given how TinkerGraph is designed. It is not really safe for multi-threaded writes (multi-threaded reads should typically not be a problem). I'd recommend that if you use Gremlin Server for testing that you switch from TinkerGraph to something more robust like neo4j, bitsy, janusgraph, etc. - i.e. a graph more suited to handling multi-threading. |
|
@spmallette well, I've just tested neo4j, bitsy and janusgraph and they don't fit: I need user provided identifiers and I need identifiers to be strings. From what I see only Neptune and Tinkergraph have full support for them |
|
even TinkerGraph isn't a good match as it doesn't support multi-request transactions, but perhaps you aren't using those. i'm not sure what else to advise at the moment i'm afraid. |
5 participants
No description provided.