You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@atlas.apache.org by Nikhil Bonte <ni...@freestoneinfotech.com> on 2020/10/28 13:59:20 UTC
Re: Review Request 72993: Cache getGuid and getStatus in
GraphTransactionInterceptor
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72993/
-----------------------------------------------------------
(Updated Oct. 28, 2020, 1:59 p.m.)
Review request for atlas, Ashutosh Mestry, Jayendra Parab, madhan, Nixon Rodrigues, Sarath Subramanian, and Sidharth Mishra.
Bugs: ATLAS-4008
https://issues.apache.org/jira/browse/ATLAS-4008
Repository: atlas
Description
-------
Problem Statement:
GraphHelper.getGuid() & GraphHelper.getStatus() causes overhead when called multiple times in a same request.
Approach:
Implement a caching mechanism in GraphTransactionInterceptor to cache the following mappings:
vertexID -> guid
vertexID -> entityStatus
Diffs
-----
repository/src/main/java/org/apache/atlas/GraphTransactionInterceptor.java 57e454a8e
repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java ca46d0fdb
repository/src/main/java/org/apache/atlas/repository/store/graph/v1/DeleteHandlerV1.java 9360dd3c9
repository/src/test/java/org/apache/atlas/repository/store/graph/v2/AtlasComplexAttributesTest.java ad5fa92bd
repository/src/test/java/org/apache/atlas/repository/store/graph/v2/AtlasEntityStoreV2Test.java b9cbef1b0
Diff: https://reviews.apache.org/r/72993/diff/1/
Testing (updated)
-------
PC build : https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/164/console - In progress
Thanks,
Nikhil Bonte
Re: Review Request 72993: Cache getGuid and getStatus in
GraphTransactionInterceptor
Posted by Nikhil Bonte <ni...@freestoneinfotech.com>.
> On Oct. 28, 2020, 4:11 p.m., Ashutosh Mestry wrote:
> > repository/src/test/java/org/apache/atlas/repository/store/graph/v2/AtlasEntityStoreV2Test.java
> > Line 860 (original), 861 (patched)
> > <https://reviews.apache.org/r/72993/diff/1/?file=2241708#file2241708line861>
> >
> > fyi: Since we use different DI mechanisms in test and regular code, the annotation does not get applied, because of which the finally block within _GraphTransactionInterceptor_ does not get called, hence explict call.
> >
> > The test executes fine since everything happens in same txn.
Thanks for the info & helping out to fix UTs.
- Nikhil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72993/#review222139
-----------------------------------------------------------
On Oct. 29, 2020, 9:34 a.m., Nikhil Bonte wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72993/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2020, 9:34 a.m.)
>
>
> Review request for atlas, Ashutosh Mestry, Jayendra Parab, madhan, Nixon Rodrigues, Sarath Subramanian, and Sidharth Mishra.
>
>
> Bugs: ATLAS-4008
> https://issues.apache.org/jira/browse/ATLAS-4008
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Problem Statement:
>
> GraphHelper.getGuid() & GraphHelper.getStatus() causes overhead when called multiple times in a same request.
>
>
>
> Approach:
>
> Implement a caching mechanism in GraphTransactionInterceptor to cache the following mappings:
>
> vertexID -> guid
>
> vertexID -> entityStatus
>
>
> Diffs
> -----
>
> repository/src/main/java/org/apache/atlas/GraphTransactionInterceptor.java 57e454a8e
> repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java ca46d0fdb
> repository/src/main/java/org/apache/atlas/repository/store/graph/v1/DeleteHandlerV1.java 9360dd3c9
> repository/src/test/java/org/apache/atlas/repository/store/graph/v2/AtlasComplexAttributesTest.java ad5fa92bd
> repository/src/test/java/org/apache/atlas/repository/store/graph/v2/AtlasEntityStoreV2Test.java b9cbef1b0
>
>
> Diff: https://reviews.apache.org/r/72993/diff/2/
>
>
> Testing
> -------
>
> PC build : https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/164/console - In progress
>
>
> Thanks,
>
> Nikhil Bonte
>
>
Re: Review Request 72993: Cache getGuid and getStatus in
GraphTransactionInterceptor
Posted by Ashutosh Mestry via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72993/#review222139
-----------------------------------------------------------
repository/src/main/java/org/apache/atlas/GraphTransactionInterceptor.java
Line 120 (original), 137 (patched)
<https://reviews.apache.org/r/72993/#comment311201>
Duplicate code: Use clearCache.
repository/src/test/java/org/apache/atlas/repository/store/graph/v2/AtlasEntityStoreV2Test.java
Line 860 (original), 861 (patched)
<https://reviews.apache.org/r/72993/#comment311202>
fyi: Since we use different DI mechanisms in test and regular code, the annotation does not get applied, because of which the finally block within _GraphTransactionInterceptor_ does not get called, hence explict call.
The test executes fine since everything happens in same txn.
- Ashutosh Mestry
On Oct. 28, 2020, 1:59 p.m., Nikhil Bonte wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72993/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2020, 1:59 p.m.)
>
>
> Review request for atlas, Ashutosh Mestry, Jayendra Parab, madhan, Nixon Rodrigues, Sarath Subramanian, and Sidharth Mishra.
>
>
> Bugs: ATLAS-4008
> https://issues.apache.org/jira/browse/ATLAS-4008
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Problem Statement:
>
> GraphHelper.getGuid() & GraphHelper.getStatus() causes overhead when called multiple times in a same request.
>
>
>
> Approach:
>
> Implement a caching mechanism in GraphTransactionInterceptor to cache the following mappings:
>
> vertexID -> guid
>
> vertexID -> entityStatus
>
>
> Diffs
> -----
>
> repository/src/main/java/org/apache/atlas/GraphTransactionInterceptor.java 57e454a8e
> repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java ca46d0fdb
> repository/src/main/java/org/apache/atlas/repository/store/graph/v1/DeleteHandlerV1.java 9360dd3c9
> repository/src/test/java/org/apache/atlas/repository/store/graph/v2/AtlasComplexAttributesTest.java ad5fa92bd
> repository/src/test/java/org/apache/atlas/repository/store/graph/v2/AtlasEntityStoreV2Test.java b9cbef1b0
>
>
> Diff: https://reviews.apache.org/r/72993/diff/1/
>
>
> Testing
> -------
>
> PC build : https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/164/console - In progress
>
>
> Thanks,
>
> Nikhil Bonte
>
>
Re: Review Request 72993: Cache getGuid and getStatus in
GraphTransactionInterceptor
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72993/#review222144
-----------------------------------------------------------
Ship it!
Ship It!
- Madhan Neethiraj
On Oct. 29, 2020, 9:34 a.m., Nikhil Bonte wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72993/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2020, 9:34 a.m.)
>
>
> Review request for atlas, Ashutosh Mestry, Jayendra Parab, madhan, Nixon Rodrigues, Sarath Subramanian, and Sidharth Mishra.
>
>
> Bugs: ATLAS-4008
> https://issues.apache.org/jira/browse/ATLAS-4008
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Problem Statement:
>
> GraphHelper.getGuid() & GraphHelper.getStatus() causes overhead when called multiple times in a same request.
>
>
>
> Approach:
>
> Implement a caching mechanism in GraphTransactionInterceptor to cache the following mappings:
>
> * vertexID -> guid
> * vertexID -> entityStatus
> * edgeId -> status
>
>
> Diffs
> -----
>
> repository/src/main/java/org/apache/atlas/GraphTransactionInterceptor.java 57e454a8e
> repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java ca46d0fdb
> repository/src/main/java/org/apache/atlas/repository/store/graph/v1/DeleteHandlerV1.java 9360dd3c9
> repository/src/test/java/org/apache/atlas/repository/store/graph/v2/AtlasComplexAttributesTest.java ad5fa92bd
> repository/src/test/java/org/apache/atlas/repository/store/graph/v2/AtlasEntityStoreV2Test.java b9cbef1b0
>
>
> Diff: https://reviews.apache.org/r/72993/diff/2/
>
>
> Testing
> -------
>
> PC build : https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/167/console - Finished: SUCCESS
>
>
> Thanks,
>
> Nikhil Bonte
>
>
Re: Review Request 72993: Cache getGuid and getStatus in
GraphTransactionInterceptor
Posted by Nixon Rodrigues <ni...@freestoneinfotech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72993/#review222147
-----------------------------------------------------------
Ship it!
Ship It!
- Nixon Rodrigues
On Oct. 29, 2020, 9:34 a.m., Nikhil Bonte wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72993/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2020, 9:34 a.m.)
>
>
> Review request for atlas, Ashutosh Mestry, Jayendra Parab, madhan, Nixon Rodrigues, Sarath Subramanian, and Sidharth Mishra.
>
>
> Bugs: ATLAS-4008
> https://issues.apache.org/jira/browse/ATLAS-4008
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Problem Statement:
>
> GraphHelper.getGuid() & GraphHelper.getStatus() causes overhead when called multiple times in a same request.
>
>
>
> Approach:
>
> Implement a caching mechanism in GraphTransactionInterceptor to cache the following mappings:
>
> * vertexID -> guid
> * vertexID -> entityStatus
> * edgeId -> status
>
>
> Diffs
> -----
>
> repository/src/main/java/org/apache/atlas/GraphTransactionInterceptor.java 57e454a8e
> repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java ca46d0fdb
> repository/src/main/java/org/apache/atlas/repository/store/graph/v1/DeleteHandlerV1.java 9360dd3c9
> repository/src/test/java/org/apache/atlas/repository/store/graph/v2/AtlasComplexAttributesTest.java ad5fa92bd
> repository/src/test/java/org/apache/atlas/repository/store/graph/v2/AtlasEntityStoreV2Test.java b9cbef1b0
>
>
> Diff: https://reviews.apache.org/r/72993/diff/2/
>
>
> Testing
> -------
>
> PC build : https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/167/console - Finished: SUCCESS
>
>
> Thanks,
>
> Nikhil Bonte
>
>
Re: Review Request 72993: Cache getGuid and getStatus in
GraphTransactionInterceptor
Posted by Nikhil Bonte <ni...@freestoneinfotech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72993/
-----------------------------------------------------------
(Updated Oct. 29, 2020, 9:34 a.m.)
Review request for atlas, Ashutosh Mestry, Jayendra Parab, madhan, Nixon Rodrigues, Sarath Subramanian, and Sidharth Mishra.
Changes
-------
Addressed review comments.
Bugs: ATLAS-4008
https://issues.apache.org/jira/browse/ATLAS-4008
Repository: atlas
Description
-------
Problem Statement:
GraphHelper.getGuid() & GraphHelper.getStatus() causes overhead when called multiple times in a same request.
Approach:
Implement a caching mechanism in GraphTransactionInterceptor to cache the following mappings:
vertexID -> guid
vertexID -> entityStatus
Diffs (updated)
-----
repository/src/main/java/org/apache/atlas/GraphTransactionInterceptor.java 57e454a8e
repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java ca46d0fdb
repository/src/main/java/org/apache/atlas/repository/store/graph/v1/DeleteHandlerV1.java 9360dd3c9
repository/src/test/java/org/apache/atlas/repository/store/graph/v2/AtlasComplexAttributesTest.java ad5fa92bd
repository/src/test/java/org/apache/atlas/repository/store/graph/v2/AtlasEntityStoreV2Test.java b9cbef1b0
Diff: https://reviews.apache.org/r/72993/diff/2/
Changes: https://reviews.apache.org/r/72993/diff/1-2/
Testing
-------
PC build : https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/164/console - In progress
Thanks,
Nikhil Bonte
Re: Review Request 72993: Cache getGuid and getStatus in
GraphTransactionInterceptor
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72993/#review222140
-----------------------------------------------------------
repository/src/main/java/org/apache/atlas/GraphTransactionInterceptor.java
Lines 241 (patched)
<https://reviews.apache.org/r/72993/#comment311203>
Will 'cache' ever be null, given initialValue() above in #61?
repository/src/main/java/org/apache/atlas/GraphTransactionInterceptor.java
Lines 247 (patched)
<https://reviews.apache.org/r/72993/#comment311205>
if 'guid' is null, consider removing from cache.
repository/src/main/java/org/apache/atlas/GraphTransactionInterceptor.java
Lines 259 (patched)
<https://reviews.apache.org/r/72993/#comment311204>
Will 'cache' ever be null, given initialValue() above in #69?
repository/src/main/java/org/apache/atlas/GraphTransactionInterceptor.java
Lines 265 (patched)
<https://reviews.apache.org/r/72993/#comment311206>
if 'status' is null, consider removing from cache.
repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java
Line 818 (original), 819 (patched)
<https://reviews.apache.org/r/72993/#comment311208>
Since caching is specifically for vertices, consider changing the argument type to AtlasVertex. With current method signature, AtlasEdge can be passed as well, and it can result in incorrect guid to be returned if a vertex with the same ID exists.
If the method is currently used get guid of an AtlasEdge, consider adding a method specifically for edge:
public static String getGuid(AtlasEdge element) {
...
}
Same applies for getStatus(AtlasElement element) method as well.
repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java
Line 819 (original), 820 (patched)
<https://reviews.apache.org/r/72993/#comment311207>
Instead of using getIdForDisplay(), consider using getId() - this will avoid overhead of converting Long to String.
Object vertexId = element.getId();
String ret = GraphTransactionInterceptor.getVertexGuidFromCache(vertexId);
if (ret == null) {
ret = element.<String>getProperty(Constants.GUID_PROPERTY_KEY, String.class);
GraphTransactionInterceptor.addToVertexGuidCache(vertexId, ret);
}
Same applies for getStatus(AtlasElement element) method as well.
- Madhan Neethiraj
On Oct. 28, 2020, 1:59 p.m., Nikhil Bonte wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72993/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2020, 1:59 p.m.)
>
>
> Review request for atlas, Ashutosh Mestry, Jayendra Parab, madhan, Nixon Rodrigues, Sarath Subramanian, and Sidharth Mishra.
>
>
> Bugs: ATLAS-4008
> https://issues.apache.org/jira/browse/ATLAS-4008
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Problem Statement:
>
> GraphHelper.getGuid() & GraphHelper.getStatus() causes overhead when called multiple times in a same request.
>
>
>
> Approach:
>
> Implement a caching mechanism in GraphTransactionInterceptor to cache the following mappings:
>
> vertexID -> guid
>
> vertexID -> entityStatus
>
>
> Diffs
> -----
>
> repository/src/main/java/org/apache/atlas/GraphTransactionInterceptor.java 57e454a8e
> repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java ca46d0fdb
> repository/src/main/java/org/apache/atlas/repository/store/graph/v1/DeleteHandlerV1.java 9360dd3c9
> repository/src/test/java/org/apache/atlas/repository/store/graph/v2/AtlasComplexAttributesTest.java ad5fa92bd
> repository/src/test/java/org/apache/atlas/repository/store/graph/v2/AtlasEntityStoreV2Test.java b9cbef1b0
>
>
> Diff: https://reviews.apache.org/r/72993/diff/1/
>
>
> Testing
> -------
>
> PC build : https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/164/console - In progress
>
>
> Thanks,
>
> Nikhil Bonte
>
>