> 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.
I thought Atlas tried to reuse soft deleted edges - so we avoid creating multiple duplicate deleted edges. > 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. Ok that makes sense. Please could you add a comment to this effect. > 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. 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. > 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: > ? 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 > 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? I suspect that many (all?) of the AtlasObjectIds are actually AtlasRelatedObjectIds - so you could cast to that and get the relationship guid. > 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. I suggest the variable name be something like elementsProcessed - so it is clearer. - David ----------------------------------------------------------- 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 > >
