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

[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_r371007320
 
 

 ##########
 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(
+          carbonProperties.getProperty(CARBON_LRU_CACHE_EXPIRATION_DURATION_IN_MINUTES));
+      if (duration < 0 || duration == 0) {
+        LOGGER.info("using default value of carbon.lru.cache.expiration.duration.in.minutes = "
+            + CarbonCommonConstants.CARBON_LRU_CACHE_EXPIRATION_DURATION_IN_MINUTES_DEFAULT);
+        carbonProperties
+            .setProperty(CarbonCommonConstants.CARBON_LOCAL_DICTIONARY_SIZE_THRESHOLD_IN_MB,
+                defaultValue);
+      } else {
+        LOGGER.info("using carbon.lru.cache.expiration.duration.in.minutes = " + duration);
+        carbonProperties
+            .setProperty(CARBON_LRU_CACHE_EXPIRATION_DURATION_IN_MINUTES,
+                Long.toString(duration));
+      }
+    } catch (Exception e) {
 
 Review comment:
   In this method, only NumberFormatException will be thrown, i dont see any other type of exception. If not instead of generalised exception, use  NumberFormatException.

----------------------------------------------------------------
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_r371041664
 
 

 ##########
 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:
   @jackylk , @ravipesala : what do you think ?
   
   @Indhumathi27 : I still think we must have **entry level expiry**, just removing all the cache after certain time is not so useful as cache need to be loaded again ?
   
   check this guava issue for the same ([here](https://github.com/google/guava/issues/1203))
   I just checked on internet and I found 2 alternatives (under apache license) that supports entry level expiry.
   a. [caffeine](https://github.com/ben-manes/caffeine) -- This is on top of guava as they support guava adaptor
   b. [expiringmap](https://github.com/jhalterman/expiringmap) -- This is widely used my many companies in their project. see [here](https://github.com/jhalterman/expiringmap/wiki/Who's-Using-ExpiringMap). I guess we can use this one.
   
   

----------------------------------------------------------------
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_r371041664
 
 

 ##########
 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:
   @jackylk , @ravipesala : what do you think ?
   
   @Indhumathi27 : I still think we must have **entry level expiry**, just removing all the cache after certain time is not so useful as cache need to be loaded again ?
   
   check this guava issue for the same ([here](https://github.com/google/guava/issues/1203))
   I just checked on internet and I found 2 alternatives (under apache license) that supports entry level expiry.
   a. [caffeine](https://github.com/ben-manes/caffeine) -- This is on top of guava as they support guava adaptor
   b. [expiringmap](https://github.com/jhalterman/expiringmap) -- This is widely used by many companies in their project. see [here](https://github.com/jhalterman/expiringmap/wiki/Who's-Using-ExpiringMap). I guess we can use this one.
   
   

----------------------------------------------------------------
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_r371065288
 
 

 ##########
 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:
   @Indhumathi27 i agree with @ajantha-bhat , may we need to keep option, or give choices to user and we should have implementations based on that like time based, size based.

----------------------------------------------------------------
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_r371613959
 
 

 ##########
 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:
   @ajantha-bhat @akashrn5 ok. I will check those caching libraries and will raise new PR.

----------------------------------------------------------------
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 closed pull request #3580: [CARBONDATA-3665] Support TimeBased Cache expiration using Guava Cache
URL: https://github.com/apache/carbondata/pull/3580
 
 
   

----------------------------------------------------------------
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