[GitHub] [carbondata] kunal642 commented on a change in pull request #3893: [CARBONDATA-3959] Added new property to set the value of executor LRU cache size to 70% of the total executor memory in Inde

2020-09-01 Thread 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:
us...@infra.apache.org

[GitHub] [carbondata] kunal642 commented on a change in pull request #3893: [CARBONDATA-3959] Added new property to set the value of executor LRU cache size to 70% of the total executor memory in Inde

2020-09-01 Thread 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:
us...@infra.apache.org

[GitHub] [carbondata] kunal642 commented on a change in pull request #3893: [CARBONDATA-3959] Added new property to set the value of executor LRU cache size to 70% of the total executor memory in Inde

2020-08-26 Thread 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:
us...@infra.apache.org

[GitHub] [carbondata] kunal642 commented on a change in pull request #3893: [CARBONDATA-3959] Added new property to set the value of executor LRU cache size to 70% of the total executor memory in Inde

2020-08-26 Thread 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:
us...@infra.apache.org