You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@carbondata.apache.org by GitBox <gi...@apache.org> on 2020/08/17 13:25:17 UTC
[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.
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:
users@infra.apache.org
[GitHub] [carbondata] Karan-c980 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 IndexServer, if executor LRU cache size is not configured.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on 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 IndexServer, if executor LRU cache size is not configured.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on 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 IndexServer, if executor LRU cache size is not configured.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] vikramahuja1001 commented on 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 IndexServer, if executor LRU cache size is not configured.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on 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 IndexServer, if executor LRU cache size is not configured.
Posted by GitBox <gi...@apache.org>.
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:
users@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 IndexServer, if executor LRU cache size is not configured.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] kunal642 commented on 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 IndexServer, if executor LRU cache size is not configured.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on 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 IndexServer, if executor LRU cache size is not configured.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on 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.
Posted by GitBox <gi...@apache.org>.
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:
users@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 IndexServer, if executor LRU cache size is not configured.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] Karan980 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 IndexServer, if executor LRU cache size is not configured.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on 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 IndexServer, if executor LRU cache size is not configured.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] kunal642 commented on 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 IndexServer, if executor LRU cache size is not configured.
Posted by GitBox <gi...@apache.org>.
kunal642 commented on pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893#issuecomment-688628672
LGTM
----------------------------------------------------------------
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:
users@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 IndexServer, if executor LRU cache size is not configured.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on 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.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] Karan-c980 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 IndexServer, if executor LRU cache size is not configured.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] asfgit closed 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 IndexServer, if executor LRU cache size is not configured.
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3893:
URL: https://github.com/apache/carbondata/pull/3893
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on 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 IndexServer, if executor LRU cache size is not configured.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] Indhumathi27 commented on 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.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] Karan980 commented on 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 IndexServer, if executor LRU cache size is not configured.
Posted by GitBox <gi...@apache.org>.
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:
users@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 IndexServer, if executor LRU cache size is not configured.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org