> 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. > > Jeff Hagelberg wrote: > 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.
... simpify the *locking* - 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 > >
