> On Dec. 6, 2017, 2:54 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java > > Lines 194 (patched) > > <https://reviews.apache.org/r/63879/diff/3/?file=1897761#file1897761line194> > > > > Minor point. Do you want to trace the label as well.
Added in revised patch > On Dec. 6, 2017, 2:54 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java > > Line 202 (original), 209 (patched) > > <https://reviews.apache.org/r/63879/diff/3/?file=1897761#file1897761line209> > > > > if yhe label does not match - presumably an edge will be returned that > > does not match with the incoming parmeters. Is this an issue? I don't think that can occur - the get of adjacent edges (immediately above) will only retrieve edges with the specified label. Does that cover your concern? > 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. I think they can be ignored. > 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? 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. > 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. Personally I quite like it the way it is - I think it is clearer. > On Dec. 6, 2017, 2:54 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java > > Line 805 (original), 856 (patched) > > <https://reviews.apache.org/r/63879/diff/3/?file=1897765#file1897765line867> > > > > I suggest putting this comment content in the Java doc for the method. Sounds a good suggestion - I'll have to get you to show me how to do that. > 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 941 (patched) > > <https://reviews.apache.org/r/63879/diff/3/?file=1897765#file1897765line955> > > > > what does Classification mean here? I don't know - it is one of the enumeration values that is not processed within this switch. > 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 964 (patched) > > <https://reviews.apache.org/r/63879/diff/3/?file=1897765#file1897765line978> > > > > I do not see these cases being handled by the code below; the comment > > implies that the code needs to handle each of these cases separately. We > > just check not empty and the index. Actually I think they are covered. If empty then we do no more processing; if not empty we check index to make sure we have not already reached the end; assuming neither empty nor off-the-end we retrieve the current edge and that may be present or not but the distinction is delegated to mapStructValue (which gets the currentEdge value in the AttributeMutationContext) > 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 ? > 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 1012 (patched) > > <https://reviews.apache.org/r/63879/diff/3/?file=1897765#file1897765line1026> > > > > I wonder whether before we start processing anything - we should check > > validate that the array of new objectids are non-null, as of objectid type > > and point to valid entities. We could then reject the request upfront > > before doing any processing. As it stands it looks like the code could do > > some graph updates and then find that there is a null or invalid array > > element - leaving the graph partially updated. > > > > I would hope that this validation should be done almost immediately > > when the rest call comes in. So we only get well formed content at this > > layer that we do not need to check for nulls, invalid types or invalid > > guids. I'm not sure I can comment on that; other than what I found during the import testing was that there were partially complete objects that only later became properly formed - i.e. when we read the remaining import data. So I get the impression that having sufficient error handling in here is necessary. It *could* be supplemented by additional barriers behind the REST interface but I think that is a separate thing. > 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 1028 (patched) > > <https://reviews.apache.org/r/63879/diff/3/?file=1897765#file1897765line1042> > > > > I sugegst putting out the tostring of the element we cannot process Added in revised patch > 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? 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? > 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 1037 (patched) > > <https://reviews.apache.org/r/63879/diff/3/?file=1897765#file1897765line1051> > > > > I think I missed how we ensure thatthe edges we chaeck are for our > > label. getArrayElementsUsingRelationship will only get edges for the appropriate label > 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? 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. - 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 > >
