[GitHub] [carbondata] Indhumathi27 opened a new pull request #3653: [WIP]ExpiringMap

[GitBox]

Indhumathi27 opened a new pull request #3653: [WIP]ExpiringMap
URL: https://github.com/apache/carbondata/pull/3653
 
 
    ### 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 #3653: [WIP]ExpiringMap
URL: https://github.com/apache/carbondata/pull/3653#issuecomment-593961774
 
 
   Build Success with Spark 2.4.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.4/586/
   

----------------------------------------------------------------
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 #3653: [WIP]ExpiringMap
URL: https://github.com/apache/carbondata/pull/3653#issuecomment-594000096
 
 
   Build Failed  with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2292/
   

----------------------------------------------------------------
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 #3653: [WIP]ExpiringMap
URL: https://github.com/apache/carbondata/pull/3653#issuecomment-594525400
 
 
   Build Success with Spark 2.4.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.4/610/
   

----------------------------------------------------------------
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 #3653: [WIP]ExpiringMap
URL: https://github.com/apache/carbondata/pull/3653#issuecomment-594528402
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2317/
   

----------------------------------------------------------------
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 #3653: [WIP]ExpiringMap
URL: https://github.com/apache/carbondata/pull/3653#issuecomment-595241447
 
 
   Build Success with Spark 2.4.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.4/640/
   

----------------------------------------------------------------
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 #3653: [WIP]ExpiringMap
URL: https://github.com/apache/carbondata/pull/3653#issuecomment-595247526
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2347/
   

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

jackylk commented on a change in pull request #3653: [CARBONDATA-3665] Support TimeBased Cache expiration using ExpiringMap
URL: https://github.com/apache/carbondata/pull/3653#discussion_r388674384
 
 

 ##########
 File path: core/pom.xml
 ##########
 @@ -59,6 +59,11 @@
       <artifactId>snappy-java</artifactId>
       <version>${snappy.version}</version>
     </dependency>
+    <dependency>
+      <groupId>net.jodah</groupId>
 
 Review comment:
   What is the license of this software?

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

 ##########
 File path: core/pom.xml
 ##########
 @@ -59,6 +59,11 @@
       <artifactId>snappy-java</artifactId>
       <version>${snappy.version}</version>
     </dependency>
+    <dependency>
+      <groupId>net.jodah</groupId>
 
 Review comment:
   Come's under Apache LIcense

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

 ##########
 File path: core/pom.xml
 ##########
 @@ -59,6 +59,11 @@
       <artifactId>snappy-java</artifactId>
       <version>${snappy.version}</version>
     </dependency>
+    <dependency>
+      <groupId>net.jodah</groupId>
 
 Review comment:
   Come's under Apache LIcense
   
   

----------------------------------------------------------------
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 #3653: [CARBONDATA-3665] Support TimeBased Cache expiration using ExpiringMap
URL: https://github.com/apache/carbondata/pull/3653#issuecomment-595707163
 
 
   Build Success with Spark 2.4.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.4/648/
   

----------------------------------------------------------------
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 #3653: [CARBONDATA-3665] Support TimeBased Cache expiration using ExpiringMap
URL: https://github.com/apache/carbondata/pull/3653#issuecomment-595709871
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2354/
   

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

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/cache/CarbonLRUCache.java
 ##########
 @@ -95,9 +97,9 @@ public CarbonLRUCache(String propertyName, String defaultPropertyName) {
    * initialize lru cache
    */
   private void initCache() {
-    lruCacheMap =
-        new LinkedHashMap<String, Cacheable>(CarbonCommonConstants.DEFAULT_COLLECTION_SIZE, 1.0f,
-            true);
+    expiringMap =
+        ExpiringMap.builder().expirationPolicy(ExpirationPolicy.ACCESSED).variableExpiration()
 
 Review comment:
   Add a comment about ExpirationPolicy.ACCESSED because expiry is based on the last access not based on the creation time.
   And setting to variable expiration as each entry can have its own expiry time.
   

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

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/cache/CarbonLRUCache.java
 ##########
 @@ -215,54 +218,27 @@ public boolean put(String columnIdentifier, Cacheable cacheInfo, long requiredSi
         }
       }
     } else {
-      synchronized (lruCacheMap) {
-        addEntryToLRUCacheMap(columnIdentifier, cacheInfo);
+      synchronized (expiringMap) {
+        addEntryToLRUCacheMap(columnIdentifier, cacheInfo, expiration_time);
         currentSize = currentSize + requiredSize;
       }
       columnKeyAddedSuccessfully = true;
     }
     return columnKeyAddedSuccessfully;
   }
 
-  /**
-   * This method will check if required size is available in the memory
-   * @param columnIdentifier
-   * @param requiredSize
-   * @return
-   */
-  public boolean tryPut(String columnIdentifier, long requiredSize) {
-    if (LOGGER.isDebugEnabled()) {
-      LOGGER.debug("checking Required size for entry " + columnIdentifier + " :: " + requiredSize
-          + " Current cache size :: " + currentSize);
-    }
-    boolean columnKeyCanBeAdded = false;
-    if (isLRUCacheSizeConfigured()) {
-      synchronized (lruCacheMap) {
-        if (freeMemorySizeForAddingCache(requiredSize)) {
-          columnKeyCanBeAdded = true;
-        } else {
-          LOGGER.error(
-              "Size check failed.Size not available. Entry cannot be added to lru cache :: "
-                  + columnIdentifier + " .Required Size = " + requiredSize + " Size available " + (
-                  lruCacheMemorySize - currentSize));
-        }
-      }
-    } else {
-      columnKeyCanBeAdded = true;
-    }
-    return columnKeyCanBeAdded;
-  }
-
   /**
    * The method will add the cache entry to LRU cache map
    *
    * @param columnIdentifier
    * @param cacheInfo
    */
-  private void addEntryToLRUCacheMap(String columnIdentifier, Cacheable cacheInfo) {
-    if (null == lruCacheMap.get(columnIdentifier)) {
-      lruCacheMap.put(columnIdentifier, cacheInfo);
-    }
+  private void addEntryToLRUCacheMap(String columnIdentifier, Cacheable cacheInfo,
+      long expiration_time) {
+    if (null == expiringMap.get(columnIdentifier) && expiration_time != 0L) {
+      expiringMap.put(columnIdentifier, cacheInfo, ExpirationPolicy.ACCESSED, expiration_time,
+          TimeUnit.MINUTES);
 
 Review comment:
   Keep default time unit in seconds, it gives fine control.

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

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/cache/CarbonLRUCache.java
 ##########
 @@ -215,54 +218,27 @@ public boolean put(String columnIdentifier, Cacheable cacheInfo, long requiredSi
         }
       }
     } else {
-      synchronized (lruCacheMap) {
-        addEntryToLRUCacheMap(columnIdentifier, cacheInfo);
+      synchronized (expiringMap) {
+        addEntryToLRUCacheMap(columnIdentifier, cacheInfo, expiration_time);
         currentSize = currentSize + requiredSize;
       }
       columnKeyAddedSuccessfully = true;
     }
     return columnKeyAddedSuccessfully;
   }
 
-  /**
-   * This method will check if required size is available in the memory
-   * @param columnIdentifier
-   * @param requiredSize
-   * @return
-   */
-  public boolean tryPut(String columnIdentifier, long requiredSize) {
-    if (LOGGER.isDebugEnabled()) {
-      LOGGER.debug("checking Required size for entry " + columnIdentifier + " :: " + requiredSize
-          + " Current cache size :: " + currentSize);
-    }
-    boolean columnKeyCanBeAdded = false;
-    if (isLRUCacheSizeConfigured()) {
-      synchronized (lruCacheMap) {
-        if (freeMemorySizeForAddingCache(requiredSize)) {
-          columnKeyCanBeAdded = true;
-        } else {
-          LOGGER.error(
-              "Size check failed.Size not available. Entry cannot be added to lru cache :: "
-                  + columnIdentifier + " .Required Size = " + requiredSize + " Size available " + (
-                  lruCacheMemorySize - currentSize));
-        }
-      }
-    } else {
-      columnKeyCanBeAdded = true;
-    }
-    return columnKeyCanBeAdded;
-  }
-
   /**
    * The method will add the cache entry to LRU cache map
    *
    * @param columnIdentifier
    * @param cacheInfo
    */
-  private void addEntryToLRUCacheMap(String columnIdentifier, Cacheable cacheInfo) {
-    if (null == lruCacheMap.get(columnIdentifier)) {
-      lruCacheMap.put(columnIdentifier, cacheInfo);
-    }
+  private void addEntryToLRUCacheMap(String columnIdentifier, Cacheable cacheInfo,
+      long expiration_time) {
+    if (null == expiringMap.get(columnIdentifier) && expiration_time != 0L) {
+      expiringMap.put(columnIdentifier, cacheInfo, ExpirationPolicy.ACCESSED, expiration_time,
+          TimeUnit.MINUTES);
+    } else expiringMap.putIfAbsent(columnIdentifier, cacheInfo);
 
 Review comment:
   Need to assign a default value and keep ExpirationPolicy.ACCESSED as policy.
   
   Also maintain {} for else

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

 ##########
 File path: integration/spark/src/main/scala/org/apache/carbondata/spark/util/CommonUtil.scala
 ##########
 @@ -416,6 +416,31 @@ object CommonUtil {
     }
   }
 
+  /**
+   * This method will validate the cache expiration time specified by the user
+   *
+   * @param tableProperties table property specified by user
+   * @param propertyName property name
+   */
+  def validateCacheExpiration(tableProperties: Map[String, String], propertyName: String): Unit = {
+    var expirationTime: java.lang.Long = 0L
+    if (tableProperties.get(propertyName).isDefined) {
+      val value = tableProperties(propertyName)
+      val exceptionMsg = s"Invalid $propertyName value found: " +
 
 Review comment:
   change this after changing unit to seconds

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

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/indexstore/BlockletDataMapIndexStore.java
 ##########
 @@ -146,8 +146,15 @@ public BlockletDataMapIndexWrapper get(TableBlockIndexUniqueIdentifierWrapper id
               new BlockletDataMapIndexWrapper(identifier.getSegmentId(), dataMaps);
         }
         if (identifierWrapper.isAddTableBlockToUnsafeAndLRUCache()) {
+          String cacheExpirationTime =
+              identifierWrapper.getCarbonTable().getTableInfo().getFactTable().getTableProperties()
 
 Review comment:
   move to method and reuse everywhere

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

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java
 ##########
 @@ -2399,4 +2399,9 @@ private CarbonCommonConstants() {
   public static final String BUCKET_COLUMNS = "bucket_columns";
   public static final String BUCKET_NUMBER = "bucket_number";
 
+  /**
+   * property for table level cache expiration
+   */
+  public static final String CACHE_EXPIRATION_TIME = "cache_expiration_time";
 
 Review comment:
   better to have self-explanatory names like this `cache_expiration_time_in_seconds`.
   Name should tell the unit.

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

 ##########
 File path: integration/spark/src/test/scala/org/apache/carbondata/sql/commands/TestCarbonShowCacheCommand.scala
 ##########
 @@ -288,4 +289,25 @@ class TestCarbonShowCacheCommand extends QueryTest with BeforeAndAfterAll {
     assert(showCache(0).get(2).toString.equalsIgnoreCase("5/5 index files cached"))
     sql("drop table if exists partitionTable")
   }
+
+  test("test cache expiration using expiringMap") {
+    sql("drop table if exists carbonTable")
+    sql("create table carbonTable(col1 int, col2 string,col3 string) stored as carbondata tblproperties('cache_expiration_time'='1')")
+    sql("insert into carbonTable select 1, 'ab', 'vf'")
+    sql("select count(*) from carbonTable").show(false)
+    var showCache = sql("show metacache on table carbonTable").collect()
+    assert(showCache(0).get(2).toString.equalsIgnoreCase("1/1 index files cached"))
+    Thread.sleep(60000)
+    showCache = sql("show metacache on table carbonTable").collect()
+    assert(showCache(0).get(2).toString.equalsIgnoreCase("0/1 index files cached"))
+  }
+
+  test("test cache expiration using expiringMap with invalid cache expiration time") {
+    sql("drop table if exists carbonTable")
+    intercept[MalformedCarbonCommandException] {
+      sql(
+        "create table carbonTable(col1 int, col2 string,col3 string) stored as carbondata tblproperties('cache_expiration_time'='ab')")
+    }.getMessage.contains("Invalid cache_expiration_time value found: ab")
+  }
 
 Review comment:
   Add tets case for bloom also

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

 ##########
 File path: integration/spark/src/test/scala/org/apache/carbondata/sql/commands/TestCarbonShowCacheCommand.scala
 ##########
 @@ -288,4 +289,25 @@ class TestCarbonShowCacheCommand extends QueryTest with BeforeAndAfterAll {
     assert(showCache(0).get(2).toString.equalsIgnoreCase("5/5 index files cached"))
     sql("drop table if exists partitionTable")
   }
+
+  test("test cache expiration using expiringMap") {
+    sql("drop table if exists carbonTable")
+    sql("create table carbonTable(col1 int, col2 string,col3 string) stored as carbondata tblproperties('cache_expiration_time'='1')")
+    sql("insert into carbonTable select 1, 'ab', 'vf'")
+    sql("select count(*) from carbonTable").show(false)
+    var showCache = sql("show metacache on table carbonTable").collect()
+    assert(showCache(0).get(2).toString.equalsIgnoreCase("1/1 index files cached"))
+    Thread.sleep(60000)
 
 Review comment:
   1 minute is too much waiting time. change to 5 seconds

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