[GitHub] [carbondata] Indhumathi27 opened a new pull request #3580: [WIP] Support TimeBased Cache expiration using Guava Cache

[GitBox]

Indhumathi27 opened a new pull request #3580: [WIP] Support TimeBased Cache expiration using Guava Cache
URL: https://github.com/apache/carbondata/pull/3580
 
 
    ### Why is this PR needed?
   
   
    ### What changes were proposed in this PR?
   
       
    ### Does this PR introduce any user interface change?
    - No
    - Yes. (please explain the change and update document)
   
    ### Is any new testcase added?
    - No
    - Yes
   
       
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3580: [WIP] Support TimeBased Cache expiration using Guava Cache
URL: https://github.com/apache/carbondata/pull/3580#issuecomment-574696831
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1646/
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3580: [WIP] Support TimeBased Cache expiration using Guava Cache
URL: https://github.com/apache/carbondata/pull/3580#issuecomment-574939662
 
 
   Build Failed  with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1647/
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

Indhumathi27 commented on issue #3580: [WIP] Support TimeBased Cache expiration using Guava Cache
URL: https://github.com/apache/carbondata/pull/3580#issuecomment-575119953
 
 
   retest this please

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3580: [WIP][CARBONDATA-3665] Support TimeBased Cache expiration using Guava Cache
URL: https://github.com/apache/carbondata/pull/3580#issuecomment-575149816
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1663/
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3580: [CARBONDATA-3665] Support TimeBased Cache expiration using Guava Cache
URL: https://github.com/apache/carbondata/pull/3580#issuecomment-575447231
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1672/
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

ajantha-bhat commented on a change in pull request #3580: [CARBONDATA-3665] Support TimeBased Cache expiration using Guava Cache
URL: https://github.com/apache/carbondata/pull/3580#discussion_r368259071
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/cache/CarbonLRUCache.java
 ##########
 @@ -43,10 +45,11 @@
   private static final Logger LOGGER =
       LogServiceFactory.getLogService(CarbonLRUCache.class.getName());
   /**
-   * Map that will contain key as table unique name and value as cache Holder
+   * Guava cache that holds key as table unique name and value as cache Holder
    * object
    */
-  private Map<String, Cacheable> lruCacheMap;
+  private Cache<String, Cacheable> lruCacheMap;
 
 Review comment:
   There are scenarios where cache of some table need to be maintained and other tables can be dropped. **It will be good if we support table level expiry.**  and not clear all the cache when expire.
   
   Drop meta cache DML is already supporting table level dropping , but it is manual work now. If this can be handled by guava, it is great

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

xuchuanyin commented on a change in pull request #3580: [CARBONDATA-3665] Support TimeBased Cache expiration using Guava Cache
URL: https://github.com/apache/carbondata/pull/3580#discussion_r368768906
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/cache/CarbonLRUCache.java
 ##########
 @@ -95,9 +98,17 @@ public CarbonLRUCache(String propertyName, String defaultPropertyName) {
    * initialize lru cache
    */
   private void initCache() {
+    // get duration for cache expiration if configured
+    long duration = CarbonCommonConstants.CARBON_LRU_CACHE_EXPIRATION_DURATION_IN_MINUTES_DEFAULT;
+    String timeBasedExpiration = CarbonProperties.getInstance()
+        .getProperty(CarbonCommonConstants.CARBON_LRU_CACHE_EXPIRATION_DURATION_IN_MINUTES);
+    if (null != timeBasedExpiration) {
+      duration = Long.parseLong(timeBasedExpiration);
 
 Review comment:
   forget to validate the content?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

xuchuanyin commented on a change in pull request #3580: [CARBONDATA-3665] Support TimeBased Cache expiration using Guava Cache
URL: https://github.com/apache/carbondata/pull/3580#discussion_r368769292
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/cache/CarbonLRUCache.java
 ##########
 @@ -95,9 +98,17 @@ public CarbonLRUCache(String propertyName, String defaultPropertyName) {
    * initialize lru cache
    */
   private void initCache() {
+    // get duration for cache expiration if configured
+    long duration = CarbonCommonConstants.CARBON_LRU_CACHE_EXPIRATION_DURATION_IN_MINUTES_DEFAULT;
+    String timeBasedExpiration = CarbonProperties.getInstance()
+        .getProperty(CarbonCommonConstants.CARBON_LRU_CACHE_EXPIRATION_DURATION_IN_MINUTES);
+    if (null != timeBasedExpiration) {
+      duration = Long.parseLong(timeBasedExpiration);
+    }
+    // initialise guava cache with time based expiration
     lruCacheMap =
-        new LinkedHashMap<String, Cacheable>(CarbonCommonConstants.DEFAULT_COLLECTION_SIZE, 1.0f,
-            true);
+        CacheBuilder.newBuilder().initialCapacity(CarbonCommonConstants.DEFAULT_COLLECTION_SIZE)
+            .expireAfterWrite(duration, TimeUnit.MINUTES).build();
 
 Review comment:
   expire after write or access?
   
   besides, guava also support size based cache, will it be supported later?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

xuchuanyin commented on a change in pull request #3580: [CARBONDATA-3665] Support TimeBased Cache expiration using Guava Cache
URL: https://github.com/apache/carbondata/pull/3580#discussion_r368770941
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/cache/CarbonLRUCache.java
 ##########
 @@ -327,15 +309,15 @@ public Cacheable get(String key) {
    */
   public void clear() {
     synchronized (lruCacheMap) {
-      for (Cacheable cachebleObj : lruCacheMap.values()) {
+      for (Cacheable cachebleObj : lruCacheMap.asMap().values()) {
 
 Review comment:
   why not use method `invalidateAll()` ?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

xuchuanyin commented on a change in pull request #3580: [CARBONDATA-3665] Support TimeBased Cache expiration using Guava Cache
URL: https://github.com/apache/carbondata/pull/3580#discussion_r368769593
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/cache/CarbonLRUCache.java
 ##########
 @@ -95,9 +98,17 @@ public CarbonLRUCache(String propertyName, String defaultPropertyName) {
    * initialize lru cache
    */
   private void initCache() {
+    // get duration for cache expiration if configured
+    long duration = CarbonCommonConstants.CARBON_LRU_CACHE_EXPIRATION_DURATION_IN_MINUTES_DEFAULT;
+    String timeBasedExpiration = CarbonProperties.getInstance()
+        .getProperty(CarbonCommonConstants.CARBON_LRU_CACHE_EXPIRATION_DURATION_IN_MINUTES);
+    if (null != timeBasedExpiration) {
+      duration = Long.parseLong(timeBasedExpiration);
+    }
+    // initialise guava cache with time based expiration
     lruCacheMap =
 
 Review comment:
   better to optimize this variable's name -- `lruCache` is enough

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

Indhumathi27 commented on a change in pull request #3580: [CARBONDATA-3665] Support TimeBased Cache expiration using Guava Cache
URL: https://github.com/apache/carbondata/pull/3580#discussion_r369371985
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/cache/CarbonLRUCache.java
 ##########
 @@ -95,9 +98,17 @@ public CarbonLRUCache(String propertyName, String defaultPropertyName) {
    * initialize lru cache
    */
   private void initCache() {
+    // get duration for cache expiration if configured
+    long duration = CarbonCommonConstants.CARBON_LRU_CACHE_EXPIRATION_DURATION_IN_MINUTES_DEFAULT;
+    String timeBasedExpiration = CarbonProperties.getInstance()
+        .getProperty(CarbonCommonConstants.CARBON_LRU_CACHE_EXPIRATION_DURATION_IN_MINUTES);
+    if (null != timeBasedExpiration) {
+      duration = Long.parseLong(timeBasedExpiration);
 
 Review comment:
   Configuration is already validated in `core/src/main/java/org/apache/carbondata/core/util/CarbonProperties.java`. Please check

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

Indhumathi27 commented on a change in pull request #3580: [CARBONDATA-3665] Support TimeBased Cache expiration using Guava Cache
URL: https://github.com/apache/carbondata/pull/3580#discussion_r369374280
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/cache/CarbonLRUCache.java
 ##########
 @@ -43,10 +45,11 @@
   private static final Logger LOGGER =
       LogServiceFactory.getLogService(CarbonLRUCache.class.getName());
   /**
-   * Map that will contain key as table unique name and value as cache Holder
+   * Guava cache that holds key as table unique name and value as cache Holder
    * object
    */
-  private Map<String, Cacheable> lruCacheMap;
+  private Cache<String, Cacheable> lruCacheMap;
 
 Review comment:
   Currently, using guava cache, we can do only time-based or size-based eviction for all values(not specific to table level) loaded to CarbonLruCache.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

Indhumathi27 commented on a change in pull request #3580: [CARBONDATA-3665] Support TimeBased Cache expiration using Guava Cache
URL: https://github.com/apache/carbondata/pull/3580#discussion_r369374454
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/cache/CarbonLRUCache.java
 ##########
 @@ -95,9 +98,17 @@ public CarbonLRUCache(String propertyName, String defaultPropertyName) {
    * initialize lru cache
    */
   private void initCache() {
+    // get duration for cache expiration if configured
+    long duration = CarbonCommonConstants.CARBON_LRU_CACHE_EXPIRATION_DURATION_IN_MINUTES_DEFAULT;
+    String timeBasedExpiration = CarbonProperties.getInstance()
+        .getProperty(CarbonCommonConstants.CARBON_LRU_CACHE_EXPIRATION_DURATION_IN_MINUTES);
+    if (null != timeBasedExpiration) {
+      duration = Long.parseLong(timeBasedExpiration);
+    }
+    // initialise guava cache with time based expiration
     lruCacheMap =
-        new LinkedHashMap<String, Cacheable>(CarbonCommonConstants.DEFAULT_COLLECTION_SIZE, 1.0f,
-            true);
+        CacheBuilder.newBuilder().initialCapacity(CarbonCommonConstants.DEFAULT_COLLECTION_SIZE)
+            .expireAfterWrite(duration, TimeUnit.MINUTES).build();
 
 Review comment:
   Size-based eviction can be supported later if needed

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

Indhumathi27 commented on a change in pull request #3580: [CARBONDATA-3665] Support TimeBased Cache expiration using Guava Cache
URL: https://github.com/apache/carbondata/pull/3580#discussion_r369387237
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/cache/CarbonLRUCache.java
 ##########
 @@ -327,15 +309,15 @@ public Cacheable get(String key) {
    */
   public void clear() {
     synchronized (lruCacheMap) {
-      for (Cacheable cachebleObj : lruCacheMap.values()) {
+      for (Cacheable cachebleObj : lruCacheMap.asMap().values()) {
 
 Review comment:
   the above code is for invalidating Cacheable objects. For LRUCache, cleanUp method is used for clearing the cache.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

Indhumathi27 commented on a change in pull request #3580: [CARBONDATA-3665] Support TimeBased Cache expiration using Guava Cache
URL: https://github.com/apache/carbondata/pull/3580#discussion_r369945607
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/cache/CarbonLRUCache.java
 ##########
 @@ -95,9 +98,17 @@ public CarbonLRUCache(String propertyName, String defaultPropertyName) {
    * initialize lru cache
    */
   private void initCache() {
+    // get duration for cache expiration if configured
+    long duration = CarbonCommonConstants.CARBON_LRU_CACHE_EXPIRATION_DURATION_IN_MINUTES_DEFAULT;
+    String timeBasedExpiration = CarbonProperties.getInstance()
+        .getProperty(CarbonCommonConstants.CARBON_LRU_CACHE_EXPIRATION_DURATION_IN_MINUTES);
+    if (null != timeBasedExpiration) {
+      duration = Long.parseLong(timeBasedExpiration);
+    }
+    // initialise guava cache with time based expiration
     lruCacheMap =
 
 Review comment:
   changed

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3580: [CARBONDATA-3665] Support TimeBased Cache expiration using Guava Cache
URL: https://github.com/apache/carbondata/pull/3580#issuecomment-577523348
 
 
   Build Failed  with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1752/
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3580: [CARBONDATA-3665] Support TimeBased Cache expiration using Guava Cache
URL: https://github.com/apache/carbondata/pull/3580#issuecomment-577554016
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1754/
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

akashrn5 commented on a change in pull request #3580: [CARBONDATA-3665] Support TimeBased Cache expiration using Guava Cache
URL: https://github.com/apache/carbondata/pull/3580#discussion_r371007560
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/cache/CarbonLRUCache.java
 ##########
 @@ -43,10 +45,11 @@
   private static final Logger LOGGER =
       LogServiceFactory.getLogService(CarbonLRUCache.class.getName());
   /**
-   * Map that will contain key as table unique name and value as cache Holder
+   * Guava cache that holds key as table unique name and value as cache Holder
    * object
    */
-  private Map<String, Cacheable> lruCacheMap;
+  private Cache<String, Cacheable> lruCache;
 
 Review comment:
   since now least recently used cache is changed to time based, does this variable name still makes sense? shall we give a better one to depict the time based cache implementation?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

akashrn5 commented on a change in pull request #3580: [CARBONDATA-3665] Support TimeBased Cache expiration using Guava Cache
URL: https://github.com/apache/carbondata/pull/3580#discussion_r371007080
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/util/CarbonProperties.java
 ##########
 @@ -1894,4 +1898,35 @@ public static String getDataMapStorageProvider() {
     }
     return provider.toUpperCase();
   }
+
+  /**
+   * This method validates the duration for carbon cache expiration
+   */
+  private void validateDurationForCacheExpiration() {
+    String defaultValue = Long.toString(
+        CarbonCommonConstants.CARBON_LRU_CACHE_EXPIRATION_DURATION_IN_MINUTES_DEFAULT);
+    long duration;
+    try {
+      duration = Long.parseLong(
 
 Review comment:
   rename to carbonCacheExpireDuration

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services