atlas-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Hagelberg <jnhagelb...@us.ibm.com>
Subject Re: Review Request 55443: ATLAS-1387 : Compiled Query Cache
Date Fri, 27 Jan 2017 21:23:36 GMT


> On Jan. 26, 2017, 9:53 p.m., Madhan Neethiraj wrote:
> > common/src/main/java/org/apache/atlas/utils/LruCache.java, line 33
> > <https://reviews.apache.org/r/55443/diff/7/?file=1616739#file1616739line33>
> >
> >     Consider using LinkedHashMap<K, V> to implement LruCache:
> >     
> >     public class LruCache<K, V> extends LinkedHashMap<K, V> {
> >       static final float LRU_LOAD_FACTOR = 0.75f;
> >     
> >       private final int maxCapacity;
> >     
> >       public LruCache(int maxCapacity) {
> >         super(maxCapacity, LRU_LOAD_FACTOR, true);
> >     
> >         this.maxCapacity = maxCapacity;
> >       }
> >     
> >       @Override
> >       protected boolean removeEldestEntry(Map.Entry eldest) {
> >          return size() > maxCapacity;
> >       }
> >     }
> >     
> >     More details at: http://docs.oracle.com/javase/7/docs/api/java/util/LinkedHashMap.html#removeEldestEntry(java.util.Map.Entry)
> 
> Jeff Hagelberg wrote:
>     I've changed the implementation of LruMap to use LinkedHashMap as the backing store.
 It would take a lot of work to make LruMap extend LinkedHashMap directly.  The main obstacle
is that some of the the methods require the last accesss time, and LinkedHashMap does not
provide that.  This at least avoids having to maintain our own linked list implementation.
> 
> Madhan Neethiraj wrote:
>     Jeff - I suggested using LinkedHashMap to avoid having to deal with complicated/error-prone
lock/unlock in our code. Is access to lastAccessTime important here, given that LinkedHashMap
automatically purges least recently accessed entries?
>     
>     The implemtation in LruCache and LruMap are not needed if LruCache extends from LinkedHashMap
as suggested in my earlier comment.

Well, the access times are only needed for the reaping functionality.  If that is not needed,
than we could probably replace LruMap with a LinkedHashMap.  LinkedHashMap does not look like
it is thread safe.  We could simplify the logging by wrapping it with a SynchronizedMap though.
 Yeah, probably the reaping is not needed, that is disabled by default anyways and really
does not add much.


- Jeff


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55443/#review163176
-----------------------------------------------------------


On Jan. 27, 2017, 5:44 p.m., Jeff Hagelberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55443/
> -----------------------------------------------------------
> 
> (Updated Jan. 27, 2017, 5:44 p.m.)
> 
> 
> Review request for atlas, David Kantor and Neeru Gupta.
> 
> 
> Bugs: ATLAS-1387
>     https://issues.apache.org/jira/browse/ATLAS-1387
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> Adds a compiled query cache to Atlas.  This avoids the overhead of parsing and translating
DSL queries that have been previously executed.
> 
> 
> Diffs
> -----
> 
>   common/pom.xml 0226541b2dd73e8a01f050982cb8b37f5fed9cab 
>   common/src/main/java/org/apache/atlas/utils/LruCache.java PRE-CREATION 
>   common/src/main/java/org/apache/atlas/utils/LruMap.java PRE-CREATION 
>   common/src/test/java/org/apache/atlas/utils/LruCacheTest.java PRE-CREATION 
>   distro/src/conf/atlas-application.properties 303ce7b00c93f2499e7b18c5f3a131b50c150b69

>   repository/src/main/java/org/apache/atlas/discovery/graph/GraphBackedDiscoveryService.java
fb488cdfc7cef195ffc5221afb9a9109a6e92fc9 
>   repository/src/main/java/org/apache/atlas/util/AtlasRepositoryConfiguration.java 6655085ee11c94addca1564cf77bbdb001c4586f

>   repository/src/main/java/org/apache/atlas/util/CompiledQueryCacheKey.java PRE-CREATION

>   repository/src/main/java/org/apache/atlas/util/NoopGremlinQuery.java PRE-CREATION 
>   repository/src/main/scala/org/apache/atlas/query/QueryProcessor.scala 5693c9ecc1a96154a8b0ac48256e5074dc041c5f

>   repository/src/test/java/org/apache/atlas/util/CompiledQueryCacheKeyTest.java PRE-CREATION

> 
> Diff: https://reviews.apache.org/r/55443/diff/
> 
> 
> Testing
> -------
> 
> Ran all atlas tests, no regressions found.  There were some tests that failed both with
and without these changes.
> 
> 
> Thanks,
> 
> Jeff Hagelberg
> 
>


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