[GitHub] [carbondata] Karan980 opened a new pull request #3893: Added new property to set the value of executor LRU cache size to 70% of the total executor memory in IndexServer, if executor LRU cache size is not configured.

[GitBox]

Karan980 opened a new pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893


    ### Why is this PR needed?
   This PR will set  executor LRU cache memory to 70% of executor memory size, if it is not configured.
   
   
    ### What changes were proposed in this PR?
   Added new property to set executor LRU cache size to 70%
   
   
    ### Does this PR introduce any user interface change?
    - No
   
    ### Is any new testcase added?
    - No
   
   
       
   


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

[GitBox]

CarbonDataQA1 commented on pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893#issuecomment-674954792


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3748/
   


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

[GitBox]

CarbonDataQA1 commented on pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893#issuecomment-674955113


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2007/
   


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

[GitBox]

Indhumathi27 commented on pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893#issuecomment-678918781


   @Karan980 Please create JIRA and update the PR description


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

[GitBox]

Karan980 commented on pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893#issuecomment-679002255


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

[GitBox]

kunal642 commented on a change in pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893#discussion_r477382736



##########
File path: integration/spark/src/main/scala/org/apache/carbondata/indexserver/IndexServer.scala
##########
@@ -243,11 +243,26 @@ object IndexServer extends ServerInterface {
   def main(args: Array[String]): Unit = {
     if (serverIp.isEmpty) {
       throw new RuntimeException(s"Please set the server IP to use Index Cache Server")
-    } else if (!isExecutorLRUConfigured) {
-      throw new RuntimeException(s"Executor LRU cache size is not set. Please set using " +
-                                 s"${ CarbonCommonConstants.CARBON_MAX_EXECUTOR_LRU_CACHE_SIZE }")
     } else {
       createCarbonSession()
+      if (!isExecutorLRUConfigured) {

Review comment:
       This is the wrong place to set the executor memory based on the %!!!
   If you set here then only the driver knows about the CARBON_MAX_EXECUTOR_LRU_CACHE_SIZE. Executor would still use the default value.
   
   Please move this to CacheProvider class.




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

[GitBox]

kunal642 commented on a change in pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893#discussion_r477384526



##########
File path: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java
##########
@@ -1280,6 +1280,11 @@ private CarbonCommonConstants() {
   public static final String CARBON_MAX_EXECUTOR_LRU_CACHE_SIZE =
       "carbon.max.executor.lru.cache.size";
 
+  /**
+   * when executor LRU cache is not configured, set it to 70% percent of executor memory size
+   */
+  public static final double CARBON_DEFAULT_EXECUTOR_LRU_CACHE_PERCENT = 0.7d;

Review comment:
       Please expose a property so that the user can set the % value that is required as the LRU cache size. And make sure that the user should not be able to set it to invalid values like "negative values, 0(0%), 1(100%) etc.




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

[GitBox]

vikramahuja1001 commented on pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893#issuecomment-681886430


   Please make the necessary changes in the index-server documentation as well for the property changes


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

[GitBox]

CarbonDataQA1 commented on pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893#issuecomment-684773131


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3944/
   


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

[GitBox]

CarbonDataQA1 commented on pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893#issuecomment-684774434


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2204/
   


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

[GitBox]

kunal642 commented on a change in pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893#discussion_r481745121



##########
File path: core/src/main/java/org/apache/carbondata/core/cache/CacheProvider.java
##########
@@ -148,19 +148,39 @@ private void createLRULevelCacheInstance() {
       carbonLRUCache = new CarbonLRUCache(CarbonCommonConstants.CARBON_MAX_DRIVER_LRU_CACHE_SIZE,
           CarbonCommonConstants.CARBON_MAX_LRU_CACHE_SIZE_DEFAULT);
     } else {
-      // if executor cache size is not configured then driver cache conf will be used
       String executorCacheSize = CarbonProperties.getInstance()
           .getProperty(CarbonCommonConstants.CARBON_MAX_EXECUTOR_LRU_CACHE_SIZE);
-      if (null != executorCacheSize) {
-        carbonLRUCache =
-            new CarbonLRUCache(CarbonCommonConstants.CARBON_MAX_EXECUTOR_LRU_CACHE_SIZE,
-                CarbonCommonConstants.CARBON_MAX_LRU_CACHE_SIZE_DEFAULT);
-      } else {
-        LOGGER.info(
-            "Executor LRU cache size not configured. Initializing with driver LRU cache size.");
-        carbonLRUCache = new CarbonLRUCache(CarbonCommonConstants.CARBON_MAX_DRIVER_LRU_CACHE_SIZE,
-            CarbonCommonConstants.CARBON_MAX_LRU_CACHE_SIZE_DEFAULT);
+      if (null == executorCacheSize) {
+        String executorCachePercent = CarbonProperties.getInstance()
+                .getProperty(CarbonCommonConstants.CARBON_EXECUTOR_LRU_CACHE_PERCENT);
+        long mSizeMB = Runtime.getRuntime().maxMemory() / (1024 * 1024);
+        long executorLruCache;
+        if (null != executorCachePercent) {
+          int percentValue = Integer.parseInt(executorCachePercent);
+          if (percentValue >= 5 && percentValue <= 95) {
+            double mPercent = (double) percentValue / 100;
+            executorLruCache = (long) (mSizeMB * mPercent);
+          }
+          else {

Review comment:
       please fix the indentation




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

[GitBox]

kunal642 commented on a change in pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893#discussion_r481745121



##########
File path: core/src/main/java/org/apache/carbondata/core/cache/CacheProvider.java
##########
@@ -148,19 +148,39 @@ private void createLRULevelCacheInstance() {
       carbonLRUCache = new CarbonLRUCache(CarbonCommonConstants.CARBON_MAX_DRIVER_LRU_CACHE_SIZE,
           CarbonCommonConstants.CARBON_MAX_LRU_CACHE_SIZE_DEFAULT);
     } else {
-      // if executor cache size is not configured then driver cache conf will be used
       String executorCacheSize = CarbonProperties.getInstance()
           .getProperty(CarbonCommonConstants.CARBON_MAX_EXECUTOR_LRU_CACHE_SIZE);
-      if (null != executorCacheSize) {
-        carbonLRUCache =
-            new CarbonLRUCache(CarbonCommonConstants.CARBON_MAX_EXECUTOR_LRU_CACHE_SIZE,
-                CarbonCommonConstants.CARBON_MAX_LRU_CACHE_SIZE_DEFAULT);
-      } else {
-        LOGGER.info(
-            "Executor LRU cache size not configured. Initializing with driver LRU cache size.");
-        carbonLRUCache = new CarbonLRUCache(CarbonCommonConstants.CARBON_MAX_DRIVER_LRU_CACHE_SIZE,
-            CarbonCommonConstants.CARBON_MAX_LRU_CACHE_SIZE_DEFAULT);
+      if (null == executorCacheSize) {
+        String executorCachePercent = CarbonProperties.getInstance()
+                .getProperty(CarbonCommonConstants.CARBON_EXECUTOR_LRU_CACHE_PERCENT);
+        long mSizeMB = Runtime.getRuntime().maxMemory() / (1024 * 1024);
+        long executorLruCache;
+        if (null != executorCachePercent) {
+          int percentValue = Integer.parseInt(executorCachePercent);
+          if (percentValue >= 5 && percentValue <= 95) {
+            double mPercent = (double) percentValue / 100;
+            executorLruCache = (long) (mSizeMB * mPercent);
+          }
+          else {

Review comment:
       move else to previous 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]

[GitBox]

Karan980 commented on a change in pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893#discussion_r481884773



##########
File path: core/src/main/java/org/apache/carbondata/core/cache/CacheProvider.java
##########
@@ -148,19 +148,39 @@ private void createLRULevelCacheInstance() {
       carbonLRUCache = new CarbonLRUCache(CarbonCommonConstants.CARBON_MAX_DRIVER_LRU_CACHE_SIZE,
           CarbonCommonConstants.CARBON_MAX_LRU_CACHE_SIZE_DEFAULT);
     } else {
-      // if executor cache size is not configured then driver cache conf will be used
       String executorCacheSize = CarbonProperties.getInstance()
           .getProperty(CarbonCommonConstants.CARBON_MAX_EXECUTOR_LRU_CACHE_SIZE);
-      if (null != executorCacheSize) {
-        carbonLRUCache =
-            new CarbonLRUCache(CarbonCommonConstants.CARBON_MAX_EXECUTOR_LRU_CACHE_SIZE,
-                CarbonCommonConstants.CARBON_MAX_LRU_CACHE_SIZE_DEFAULT);
-      } else {
-        LOGGER.info(
-            "Executor LRU cache size not configured. Initializing with driver LRU cache size.");
-        carbonLRUCache = new CarbonLRUCache(CarbonCommonConstants.CARBON_MAX_DRIVER_LRU_CACHE_SIZE,
-            CarbonCommonConstants.CARBON_MAX_LRU_CACHE_SIZE_DEFAULT);
+      if (null == executorCacheSize) {
+        String executorCachePercent = CarbonProperties.getInstance()
+                .getProperty(CarbonCommonConstants.CARBON_EXECUTOR_LRU_CACHE_PERCENT);
+        long mSizeMB = Runtime.getRuntime().maxMemory() / (1024 * 1024);
+        long executorLruCache;
+        if (null != executorCachePercent) {
+          int percentValue = Integer.parseInt(executorCachePercent);
+          if (percentValue >= 5 && percentValue <= 95) {
+            double mPercent = (double) percentValue / 100;
+            executorLruCache = (long) (mSizeMB * mPercent);
+          }
+          else {

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]

[GitBox]

CarbonDataQA1 commented on pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893#issuecomment-685574473


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3961/
   


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

[GitBox]

CarbonDataQA1 commented on pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893#issuecomment-685577753


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2221/
   


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

[GitBox]

kunal642 commented on pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893#issuecomment-688460202


   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]

[GitBox]

Karan-c980 commented on a change in pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893#discussion_r484537405



##########
File path: integration/spark/src/main/scala/org/apache/carbondata/indexserver/IndexServer.scala
##########
@@ -243,11 +243,26 @@ object IndexServer extends ServerInterface {
   def main(args: Array[String]): Unit = {
     if (serverIp.isEmpty) {
       throw new RuntimeException(s"Please set the server IP to use Index Cache Server")
-    } else if (!isExecutorLRUConfigured) {
-      throw new RuntimeException(s"Executor LRU cache size is not set. Please set using " +
-                                 s"${ CarbonCommonConstants.CARBON_MAX_EXECUTOR_LRU_CACHE_SIZE }")
     } else {
       createCarbonSession()
+      if (!isExecutorLRUConfigured) {

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]

[GitBox]

Karan-c980 commented on a change in pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893#discussion_r484537422



##########
File path: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java
##########
@@ -1280,6 +1280,11 @@ private CarbonCommonConstants() {
   public static final String CARBON_MAX_EXECUTOR_LRU_CACHE_SIZE =
       "carbon.max.executor.lru.cache.size";
 
+  /**
+   * when executor LRU cache is not configured, set it to 70% percent of executor memory size
+   */
+  public static final double CARBON_DEFAULT_EXECUTOR_LRU_CACHE_PERCENT = 0.7d;

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]

[GitBox]

CarbonDataQA1 commented on pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893#issuecomment-688493878


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2253/
   


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

[GitBox]

CarbonDataQA1 commented on pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893#issuecomment-688493902


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3993/
   


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