> On Dec. 6, 2017, 2:54 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java > > Line 204 (original), 211 (patched) > > <https://reviews.apache.org/r/63879/diff/3/?file=1897761#file1897761line211> > > > > what happens for delete edges. > > Graham Wallis wrote: > I think they can be ignored. > > David Radley wrote: > I thought Atlas tried to reuse soft deleted edges - so we avoid creating > multiple duplicate deleted edges.
Good point - but from a quick look through the code - particularly the handling of relationship edges, I suspect that this is a much broader problem. For example in AtlasRelationshipStoreV1.createRelationship(), getRelationshipEdge() is called to determine whether there is an existing edge; if an edge is returned then it is assumed that therelationship already exists and an exception is thron; However, if you dig into the call stack it appears (see getRelationshipEdge and getEdgeForLabel) that a soft-deleted edge can be returned. So I think you have highlighted a genuine problem, but I also think it should maybe be the subject of a separate JIRA as I suspect it affects a broader set of code than this patch (which set out to fix the pre-existing behaviour in which a search over existing elements was abandoned prematurely dependeing on the order of the target related entities). I think a separate JIRA may be needed that holistically covers the resurrection of soft-deleted objects. > On Dec. 6, 2017, 2:54 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityGraphDiscoveryV1.java > > Lines 364 (patched) > > <https://reviews.apache.org/r/63879/diff/3/?file=1897762#file1897762line369> > > > > I do not understand this comment or the following code line. What does > > "attribute is a relationship " mean? We are looping through the possible > > relationship attributes as defined in the entityType and we find that the > > entity does not have this relationship attribute. Why does > > entity.getAttribute(attrName); give us a value when > > entity.hasRelationshipAttribute(attrName) does not? > > Graham Wallis wrote: > This was to cater for what happens during import service, when you get a > partial entity. i.e. you could be reading an entity that *should* have a > reference to another but you haven't read the other entity yet. So you have a > kind of dangling, incomplete reference. You can find out from the entityType > that it should have a relationship attribute, but if you try to use > getRelationshipAttribute() to read it you will not get it. But knowing from > the type that this is a relationship, we can use the more general > getAttribute(). If you don't do this then the result is the object reference > will not be recorded, which means that the entity store will not do a > preCreate and the entity graph mapper will not be able to find the attribute > vertex, so it barfs and gives up. > > David Radley wrote: > Ok that makes sense. Please could you add a comment to this effect. Done > On Dec. 6, 2017, 2:54 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityGraphDiscoveryV1.java > > Lines 370 (patched) > > <https://reviews.apache.org/r/63879/diff/3/?file=1897762#file1897762line375> > > > > can we move this line and the line below outside the if else- as they > > are repeated in each section. > > Graham Wallis wrote: > Personally I quite like it the way it is - I think it is clearer. > > David Radley wrote: > It is duplicated code, it should be clearer and simpler to have the code > once. When I looked at the code I assumed there was some subtle different > between the lines that I was missing - so I found it took me more time to > read. I do not feel strongly about style - so if you decide to drop the > issue, that is fine by me. Well, I changed it anyway - I was feeling kind ;-) > On Dec. 6, 2017, 2:54 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java > > Lines 965 (patched) > > <https://reviews.apache.org/r/63879/diff/3/?file=1897765#file1897765line979> > > > > or empty > > Graham Wallis wrote: > ? > > David Radley wrote: > The comment talks of checking null, but the code checks for it being > empty. I suggest adding "or empty" to the comment to keep it in line withe > the code The CollectionUtils test for non-emptiness checks both - something is deemed to be not empty if it is neither null nor (non-null but) empty. > On Dec. 6, 2017, 2:54 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java > > Lines 1033 (patched) > > <https://reviews.apache.org/r/63879/diff/3/?file=1897765#file1897765line1047> > > > > could we check the relationship guid? If this was specified in the new > > elements we should match it. Or can this not be specified? > > Graham Wallis wrote: > I don't know - it is not obvious to me that that's possible; I was seeing > entity guids, types and unique attributes in the AtlasObjectIds in the > mutation contexts. Is it possible to retrieve a relationship GUID as well? > > David Radley wrote: > I suspect that many (all?) of the AtlasObjectIds are actually > AtlasRelatedObjectIds - so you could cast to that and get the relationship > guid. I can see how to get the relaionshipGuid from the new element (an Atlas[Related]ObjectId) - and have added code to extract that and log it - but it is not clear to me how to retrieve the comparative relationshipGuid of the existing edge, without navigating via the vertices to locate the relationship - and we already check the vertex guids. Interestingly - I tested with the above logging of the relationshipGuid of the new element - and the element appears to be not an AtlasRelatedObjectId. (It is an instance of AtlasObjectId). > On Dec. 6, 2017, 2:54 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java > > Lines 1146 (patched) > > <https://reviews.apache.org/r/63879/diff/3/?file=1897765#file1897765line1160> > > > > I do not understand what this if doing. it looks like the delete > > handler has done a soft delete- why would a soft deleted edgebe added to > > newElementsCreated? > > Graham Wallis wrote: > Because it adds to the overall number of elements processed; some of the > original variable names are a bit odd but I was trying to minimise changes. > > David Radley wrote: > I suggest the variable name be something like elementsProcessed - so it > is clearer. I agree - but this was one of the existing variables and I am loath to change it. - Graham ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63879/#review192983 ----------------------------------------------------------- On Nov. 20, 2017, 5:15 p.m., Graham Wallis wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63879/ > ----------------------------------------------------------- > > (Updated Nov. 20, 2017, 5:15 p.m.) > > > Review request for atlas. > > > Repository: atlas > > > Description > ------- > > ATLAS-2262.patch > This patch contains fixes for the EntityGraphMapper and related classes, to > fix intermittent relationship failures. > This change means that an UPDATE operation will inspect the existing and new > relationships and compare them fully before adding/modifying edges. Matching > elements will be reused, new elements will be added, and redundant elements > will be discarded. The behaviour should be consistent regardless of which > graph provider is being used. > > > Diffs > ----- > > repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java > 3e602431400677cbe0d8fe440732b02bb4a30a62 > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityGraphDiscoveryV1.java > 739d610263a1b5bb8a0a9ff8183196ebe2c6294b > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java > b0f9845d6e92a6ef4bb53cccec537a0a54afb5e8 > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AttributeMutationContext.java > b6d82dd834b8cc8b3bac630c084b91ac34dd0cec > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java > f6a15b695e676bc035ce8336bcce7bacf1852ff9 > > repository/src/test/java/org/apache/atlas/repository/impexp/ImportServiceTest.java > b24774d6376765f054022b9220b61769505e488c > > repository/src/test/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1Test.java > fd1b6db0c4dd57cd70ba56f49b9c4751f6915858 > > repository/src/test/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1Test.java > d207a6958ab9dce0bd9f8e08c8e49232ab7d2e8a > > > Diff: https://reviews.apache.org/r/63879/diff/3/ > > > Testing > ------- > > Build with full tests with janus grap provider > > > Thanks, > > Graham Wallis > >
