> On Dec. 15, 2017, 3:28 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java > > Line 205 (original), 208 (patched) > > <https://reviews.apache.org/r/63879/diff/3-4/?file=1897761#file1897761line209> > > > > The called method itself already traces this. The => is misleading as > > it is not an entry. I suggest removing this trace entry
The called method only traces extry and exit - it would be cumbersome/disruptive to add trace to that method since it constructs the whole iterator in the return statement; hence trace of the returned result in the caller. However, I have reduced the amount of trace in the caller which now just reports whether it is using an existing edge or adding one. > On Dec. 15, 2017, 3:28 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java > > Lines 289 (patched) > > <https://reviews.apache.org/r/63879/diff/3-4/?file=1897761#file1897761line290> > > > > I suggest you trace args - as they could be very good for debugging Added args > On Dec. 15, 2017, 3:28 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java > > Lines 524 (patched) > > <https://reviews.apache.org/r/63879/diff/3-4/?file=1897761#file1897761line526> > > > > How aboiut putting put the clazz name here? OK - I have added clazz.getName() to what is traced. > On Dec. 15, 2017, 3:28 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java > > Line 521 (original), 570 (patched) > > <https://reviews.apache.org/r/63879/diff/3-4/?file=1897761#file1897761line572> > > > > A minor suggestion : maybe "from edge {}" Done > On Dec. 15, 2017, 3:28 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java > > Line 570 (original), 630 (patched) > > <https://reviews.apache.org/r/63879/diff/3-4/?file=1897761#file1897761line633> > > > > edge => edgeString I was deliberately tracing the actual edge (AtlasEdge object) rather than the string rendering of the edge id - which is traced immediately before. Having the actual edge ref is useful for debugging. > On Dec. 15, 2017, 3:28 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java > > Lines 641 (patched) > > <https://reviews.apache.org/r/63879/diff/3-4/?file=1897761#file1897761line644> > > > > should be use the vertexString method here? I was deliberately tracing the actual vertex (AtlasVertex object) rather than the string rendering of the vertex id - which is traced just afterwards. Having the actual vertex ref is useful for debugging. > On Dec. 15, 2017, 3:28 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java > > Line 591 (original), 653 (patched) > > <https://reviews.apache.org/r/63879/diff/3-4/?file=1897761#file1897761line657> > > > > and here as above > On Dec. 15, 2017, 3:28 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java > > Line 764 (original), 833 (patched) > > <https://reviews.apache.org/r/63879/diff/3-4/?file=1897761#file1897761line837> > > > > can we put the classType out here? Originally I wasn't sure what to trace here - I have now added classType.getTypeName(), i.e. using the getTypeName() of the AtlasType from which the AtlasEntityType inherits (via AtlasSTructType). > On Dec. 15, 2017, 3:28 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityGraphDiscoveryV1.java > > Line 364 (original), 361 (patched) > > <https://reviews.apache.org/r/63879/diff/3-4/?file=1897762#file1897762line364> > > > > I suggest using block comment rather than lots of line comments. Done > On Dec. 15, 2017, 3:28 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityGraphDiscoveryV1.java > > Lines 365 (patched) > > <https://reviews.apache.org/r/63879/diff/3-4/?file=1897762#file1897762line368> > > > > I hada go at reqeriting to remove the yous from the description. what > > do you think of: > > > > "This code needs to cater for imports, where entities can contain > > attributes that refer to entities that have not been processed yet by the > > import. In this situation a partial entity comes into this method; the > > partial entity is not pully formed becase it has a reference to an entity > > that the import has not seen yet. In this case getRelationshipAttributes() > > does not get the relationshipattributes; the code needs to use the more > > general getAttribute() instead. Thanks - added something akin to what you have here. > On Dec. 15, 2017, 3:28 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AttributeMutationContext.java > > Lines 91 (patched) > > <https://reviews.apache.org/r/63879/diff/3-4/?file=1897764#file1897764line91> > > > > shouldnt we use vertexString() here? For debug it is more useful to have the whole object > On Dec. 15, 2017, 3:28 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AttributeMutationContext.java > > Lines 95 (patched) > > <https://reviews.apache.org/r/63879/diff/3-4/?file=1897764#file1897764line95> > > > > shouldnt we use edgeString() here? For debug it is more useful to have the whole object > On Dec. 15, 2017, 3:28 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java > > Lines 866 (patched) > > <https://reviews.apache.org/r/63879/diff/3-4/?file=1897765#file1897765line866> > > > > should we mention the AtlasRelatedObjectId processing here? How do you mean? From the testing/debugging I did I generally saw AtlasObjectIds here. > On Dec. 15, 2017, 3:28 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java > > Lines 1029 (patched) > > <https://reviews.apache.org/r/63879/diff/3-4/?file=1897765#file1897765line1049> > > > > If we have a relationship guid - we trace it. I was expecting it to be > > involved with the matching of new and existing edges. I added the trace to find out whether we were ever getting a relationship guid - but never saw one. I left the trace in for good measure, but decided not to add a relationshipGuid comparison. - Graham ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63879/#review193913 ----------------------------------------------------------- On Dec. 8, 2017, 12:53 p.m., Graham Wallis wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63879/ > ----------------------------------------------------------- > > (Updated Dec. 8, 2017, 12:53 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 > d6effbf77b7dbd3479bab3899730bdcbabdce351 > > 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 > ab25faaa9caffe5bf2796194057f0556d4bfc431 > > repository/src/test/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1Test.java > 1f8b9fdbfd5d987288983f7be99ea0f36f7255d4 > > repository/src/test/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1Test.java > b418fef90a0bcadba47679028c134b836bdd9079 > > > Diff: https://reviews.apache.org/r/63879/diff/4/ > > > Testing > ------- > > Build with test enabled, using Janus graph provider > > > Thanks, > > Graham Wallis > >
