You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@atlas.apache.org by Shwetha GS <ss...@hortonworks.com> on 2016/05/12 11:37:37 UTC
Re: Review Request 45948: Atlas-645: avoid infinite recursion in
FieldMapping.output()
> On April 19, 2016, 11 a.m., Shwetha GS wrote:
> > typesystem/src/main/java/org/apache/atlas/typesystem/types/FieldMapping.java, line 99
> > <https://reviews.apache.org/r/45948/diff/3/?file=1348566#file1348566line99>
> >
> > Can we maintain this as Set<String> and check on just typeName?
>
> David Kantor wrote:
> I don't understand how checking typeName would avoid the infinite recursion. The problem use case is when structs reference each other. So let's say there is a bi-directional reference between A and B. When A is output, its attribute values are output. So then B is output, and when its attributes are output, FieldMapping.output() is called recursively on A. We need to detect that A is already in the process of being output to avoid the infinite recursion. How does checking typeName help with this? Please advise.
>
> Shwetha GS wrote:
> In FieldMappingTest.testOutputReferenceableInstance(), even ownerType.toString() throws stack overflow. So, thought you were addressing type.tostring()
>
> For type.toString(), we can use typename. For instance.tostring(), can we use id string, instead of full instance in the set
>
> David Kantor wrote:
> Thanks for pointing out the stack overflow on ownerType.toString(). This involves type system objects and does not go through the FieldMapping.output() and IDataType.output() code path for outputting instance data - it takes a completely different code path. So changing the "in process" arg on these methods to Set<String> would not address this bug. Here is the relevant call stack:
>
> ClassType(HierarchicalType<ST,T>).toString() line: 371
> String.valueOf(Object) line: 1657
> StringBuilder.append(Object) line: 194
> AttributeInfo.toString() line: 65
> SingletonImmutableList<E>.toString() line: 97
> String.valueOf(Object) line: 1657
> StringBuilder.append(Object) line: 194
> ClassType(HierarchicalType<ST,T>).toString() line: 372
> String.valueOf(Object) line: 1657
> StringBuilder.append(Object) line: 194
> AttributeInfo.toString() line: 65
> SingletonImmutableList<E>.toString() line: 97
> String.valueOf(Object) line: 1657
> StringBuilder.append(Object) line: 194
> ClassType(HierarchicalType<ST,T>).toString() line: 372
>
> Also, for instance data, using the id string is not sufficient because structs that are not implementations of IReferenceableInstance won't have an id string. And for those structs that are IReferenceableInstance objects, if the structs are newly created in memory and have not been persisted, they won't have a meaningful id string. Such objects would all have the same id string, and result in false positives on the inProcess check.
>
> I have re-implemented the fix to maintain the inProcess set in thread local storage rather than passing it as an argument. This fix addresses the recursion issue for both types and instance data.
Sorry, it took long to get to this.
About thread local variables:
I think passing inProgress as argument is a better approach \u2013 it makes the interfaces clear (as arguments are explicit) and avoids global state. Thread local variables are bad idea as its global state and there is no control on who can modify it. With the application server like atlas where server threads are re-used after request completes, there should be cleanup of thread local variables for every request which is an extra overhead.
Earlier, I added RequestContext as a thread local variable because I couldn\u2019t find a cleaner way to set this for every request and pass the user info from filters to the APIs. We reinitisalise RequestContext for every request in AuditFilter. Also, if its called from other threads outside of APIs, it needs to be handled separately. Its a pain, and I need to clean that up as well.
I guess the reason why you added thread local variable is for toString() methods as it doesn't accept any arguments. Can toString() methods use output() methods in turn(like in instance classes) and pass the inProgress to output()?
About inProgress set:
For types, typesInProgress can be set of type names, so set<string> should work.
- Shwetha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45948/#review129505
-----------------------------------------------------------
On April 29, 2016, 7:24 p.m., David Kantor wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45948/
> -----------------------------------------------------------
>
> (Updated April 29, 2016, 7:24 p.m.)
>
>
> Review request for atlas.
>
>
> Bugs: ATLAS-645
> https://issues.apache.org/jira/browse/ATLAS-645
>
>
> Repository: atlas
>
>
> Description
> -------
>
> ATLAS-645: In FieldMapping.output(), avoid infinite recursion when IReferenceableInstance and IStruct instances reference each other.
>
>
> Diffs
> -----
>
> typesystem/src/main/java/org/apache/atlas/typesystem/persistence/StructInstance.java af62442bfe5daa221079207acf361e1316cab3ad
> typesystem/src/main/java/org/apache/atlas/typesystem/types/FieldMapping.java 36149bafff80b68ce176e82dcacac87035459362
> typesystem/src/main/java/org/apache/atlas/typesystem/types/HierarchicalType.java 89fcea6828c9e23c28a60952c3e4ca27c0667494
> typesystem/src/main/java/org/apache/atlas/typesystem/types/OutputStatus.java PRE-CREATION
> typesystem/src/main/java/org/apache/atlas/typesystem/types/StructType.java 54e344f5d6322a00ac7825ee8964f43a1552dcbe
> typesystem/src/test/java/org/apache/atlas/typesystem/types/OutputStatusTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45948/diff/
>
>
> Testing
> -------
>
> Ran all unit and integration tests with no regressions. Added test cases
>
>
> Thanks,
>
> David Kantor
>
>