atlas-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Radley <david...@apache.org>
Subject Re: Review Request 63879: ATLAS-2262 Fix intermittent relationship failures
Date Thu, 07 Dec 2017 10:00:43 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message