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

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

 ##########
 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:
   added

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

 ##########
 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:
   done

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

 ##########
 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
+   */
 
 Review comment:
   done

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

 ##########
 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: " +
+                         s"$value, only duration from 1 minute to 1440 minutes is supported."
+      try {
+        expirationTime = java.lang.Long.parseLong(value)
+      } catch {
+        case n: NumberFormatException =>
+          throw new MalformedCarbonCommandException(exceptionMsg)
+      }
+      if (expirationTime == 0L) {
+        throw new MalformedCarbonCommandException(exceptionMsg)
+      }
+      tableProperties.put(propertyName, value)
 
 Review comment:
   done

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

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/cache/CarbonLRUCache.java
 ##########
 @@ -215,54 +221,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) {
 
 Review comment:
   ```suggestion
         long expirationTimeSeconds) {
   ```

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

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java
 ##########
 @@ -2406,4 +2406,15 @@ private CarbonCommonConstants() {
   public static final String BUCKET_COLUMNS = "bucket_columns";
   public static final String BUCKET_NUMBER = "bucket_number";
 
+  /**
+   * property for table level cache expiration
 
 Review comment:
   ```suggestion
      * table property name for table level cache expiration. Carbon maintains index cache in driver side and the cache will be expired after seconds indicated by this table property
   ```

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

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java
 ##########
 @@ -2406,4 +2406,15 @@ private CarbonCommonConstants() {
   public static final String BUCKET_COLUMNS = "bucket_columns";
   public static final String BUCKET_NUMBER = "bucket_number";
 
+  /**
+   * property for table level cache expiration
 
 Review comment:
   ```suggestion
      * Table property name for table level cache expiration. Carbon maintains index cache in driver side and the cache will be expired after seconds indicated by this table property
   ```

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

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java
 ##########
 @@ -2406,4 +2406,15 @@ 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_IN_SECONDS = "cache_expiration_time_in_seconds";
+
+  /**
+   * default value for table level cache expiration
 
 Review comment:
   ```suggestion
      * By default, the index cache is not expired by time, thus the cache size is controlled by setting the maximum size (add the property name here)
   ```

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

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java
 ##########
 @@ -2406,4 +2406,15 @@ private CarbonCommonConstants() {
   public static final String BUCKET_COLUMNS = "bucket_columns";
   public static final String BUCKET_NUMBER = "bucket_number";
 
+  /**
+   * property for table level cache expiration
 
 Review comment:
   I think more description need to add to describe the expiration by time and by maximum size

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

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/datamap/TableDataMap.java
 ##########
 @@ -202,8 +202,9 @@ public CarbonTable getTable() {
                 null, table.getMinMaxCacheColumns(segmentProperties),
                 false);
         for (DataMap dataMap : dataMaps.get(segment)) {
-          pruneBlocklets.addAll(
-              dataMap.prune(filter.getResolver(), segmentProperties, partitions, filterExecuter));
+          pruneBlocklets.addAll(dataMap
 
 Review comment:
   make format properly, move datamap to next line

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

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java
 ##########
 @@ -2406,4 +2406,15 @@ 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_IN_SECONDS = "cache_expiration_time_in_seconds";
 
 Review comment:
   ```suggestion
     public static final String INDEX_CACHE_EXPIRATION_TIME_IN_SECONDS = "index_cache_expiration_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]

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_r392278215
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java
 ##########
 @@ -2406,4 +2406,15 @@ 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_IN_SECONDS = "cache_expiration_time_in_seconds";
 
 Review comment:
   done

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

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/datamap/TableDataMap.java
 ##########
 @@ -202,8 +202,9 @@ public CarbonTable getTable() {
                 null, table.getMinMaxCacheColumns(segmentProperties),
                 false);
         for (DataMap dataMap : dataMaps.get(segment)) {
-          pruneBlocklets.addAll(
-              dataMap.prune(filter.getResolver(), segmentProperties, partitions, filterExecuter));
+          pruneBlocklets.addAll(dataMap
 
 Review comment:
   done

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

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java
 ##########
 @@ -2406,4 +2406,15 @@ 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_IN_SECONDS = "cache_expiration_time_in_seconds";
+
+  /**
+   * default value for table level cache expiration
 
 Review comment:
   done

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

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java
 ##########
 @@ -2406,4 +2406,15 @@ private CarbonCommonConstants() {
   public static final String BUCKET_COLUMNS = "bucket_columns";
   public static final String BUCKET_NUMBER = "bucket_number";
 
+  /**
+   * property for table level cache expiration
 
 Review comment:
   added

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

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/cache/CarbonLRUCache.java
 ##########
 @@ -215,54 +221,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) {
 
 Review comment:
   done

----------------------------------------------------------------
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-598832888
 
 
   Build Failed  with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2461/
   

----------------------------------------------------------------
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-598837458
 
 
   Build Failed  with Spark 2.4.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.4/753/
   

----------------------------------------------------------------
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-599020369
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2468/
   

----------------------------------------------------------------
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-599023967
 
 
   Build Success with Spark 2.4.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.4/760/
   

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