You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/04/06 12:23:09 UTC
[GitHub] [spark] weixiuli opened a new pull request, #36088: [SPARK-38805][SHUFFLE] Automatically remove an expired indexFilePath from the ESS shuffleIndexCache or the PBS indexCache to save memory.
weixiuli opened a new pull request, #36088:
URL: https://github.com/apache/spark/pull/36088
### What changes were proposed in this pull request?
Currently, we use Guava Cache either in the ExternalShuffleBlockResolver or the RemoteBlockPushResolver to cache index file information so that we can avoid open/close the index files for each request. However, it will take up a fixed amount of memory, even though we no longer need any cached index files, which is a waste of memory.
In our production，we set spark.shuffle.service.index.cache.size to be 400m，and found above problem. So, we should automatically remove an expired indexFilePath from the ESS shuffleIndexCache or the PBS indexCache to save memory.
### Why are the changes needed?
To save memory
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing unittests and Passed CI.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] weixiuli commented on a diff in pull request #36088: [SPARK-38805][SHUFFLE] Automatically remove an expired indexFilePath from the ESS shuffleIndexCache or the PBS indexCache to save memory.
Posted by GitBox <gi...@apache.org>.
weixiuli commented on code in PR #36088:
URL: https://github.com/apache/spark/pull/36088#discussion_r847269984
##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolver.java:
##########
@@ -118,11 +119,18 @@ public ShuffleIndexInformation load(String filePath) throws IOException {
return new ShuffleIndexInformation(filePath);
}
};
- shuffleIndexCache = CacheBuilder.newBuilder()
- .maximumWeight(JavaUtils.byteStringAsBytes(indexCacheSize))
- .weigher((Weigher<String, ShuffleIndexInformation>)
- (filePath, indexInfo) -> indexInfo.getRetainedMemorySize())
- .build(indexCacheLoader);
+ CacheBuilder cacheBuilder = CacheBuilder.newBuilder()
+ .maximumWeight(JavaUtils.byteStringAsBytes(indexCacheSize))
+ .weigher((Weigher<String, ShuffleIndexInformation>)
+ (filePath, indexInfo) -> indexInfo.getRetainedMemorySize());
+ int expireTimeSeconds = conf.shuffleIndexCacheExpireTimeSeconds();
+ if (expireTimeSeconds > 0) {
+ shuffleIndexCache = cacheBuilder.expireAfterAccess(expireTimeSeconds, TimeUnit.SECONDS)
Review Comment:
A small value of expireTimeSeconds is not what we want, and we can make sure it is greater than a fixed value (such as 300s)?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on pull request #36088: [SPARK-38805][SHUFFLE] Automatically remove an expired indexFilePath from the ESS shuffleIndexCache or the PBS indexCache to save memory.
Posted by GitBox <gi...@apache.org>.
mridulm commented on PR #36088:
URL: https://github.com/apache/spark/pull/36088#issuecomment-1096126997
+CC @attilapiros
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] weixiuli commented on pull request #36088: [SPARK-38805][SHUFFLE] Automatically remove an expired indexFilePath from the ESS shuffleIndexCache or the PBS indexCache to save memory.
Posted by GitBox <gi...@apache.org>.
weixiuli commented on PR #36088:
URL: https://github.com/apache/spark/pull/36088#issuecomment-1101293957
@mridulm Thanks for your revicew. In my opinion，this PR is different from `spark.shuffle.service.index.cache.size`, It not only saves memory, but also benefits GC. We don't need cache the index file paths if we don't use them anymore.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #36088: [SPARK-38805][SHUFFLE] Automatically remove an expired indexFilePath from the ESS shuffleIndexCache or the PBS indexCache to save memory.
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #36088:
URL: https://github.com/apache/spark/pull/36088#issuecomment-1090400510
Can one of the admins verify this patch?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on pull request #36088: [SPARK-38805][SHUFFLE] Automatically remove an expired indexFilePath from the ESS shuffleIndexCache or the PBS indexCache to save memory.
Posted by GitBox <gi...@apache.org>.
mridulm commented on PR #36088:
URL: https://github.com/apache/spark/pull/36088#issuecomment-1097005678
`spark.shuffle.service.index.cache.size` is explicitly provided to control this size.
Not sure what the concern here is.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on pull request #36088: [SPARK-38805][SHUFFLE] Automatically remove an expired indexFilePath from the ESS shuffleIndexCache or the PBS indexCache to save memory.
Posted by GitBox <gi...@apache.org>.
mridulm commented on PR #36088:
URL: https://github.com/apache/spark/pull/36088#issuecomment-1096126276
The size should be bounded by `spark.shuffle.service.index.cache.size`; with the cache evicting entries based on LRU.
Did you try configuring that to control the memory footprint instead ?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] weixiuli commented on pull request #36088: [SPARK-38805][SHUFFLE] Automatically remove an expired indexFilePath from the ESS shuffleIndexCache or the PBS indexCache to save memory.
Posted by GitBox <gi...@apache.org>.
weixiuli commented on PR #36088:
URL: https://github.com/apache/spark/pull/36088#issuecomment-1094680578
cc @Ngone51 @mridulm
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on a diff in pull request #36088: [SPARK-38805][SHUFFLE] Automatically remove an expired indexFilePath from the ESS shuffleIndexCache or the PBS indexCache to save memory.
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on code in PR #36088:
URL: https://github.com/apache/spark/pull/36088#discussion_r847255904
##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolver.java:
##########
@@ -118,11 +119,18 @@ public ShuffleIndexInformation load(String filePath) throws IOException {
return new ShuffleIndexInformation(filePath);
}
};
- shuffleIndexCache = CacheBuilder.newBuilder()
- .maximumWeight(JavaUtils.byteStringAsBytes(indexCacheSize))
- .weigher((Weigher<String, ShuffleIndexInformation>)
- (filePath, indexInfo) -> indexInfo.getRetainedMemorySize())
- .build(indexCacheLoader);
+ CacheBuilder cacheBuilder = CacheBuilder.newBuilder()
+ .maximumWeight(JavaUtils.byteStringAsBytes(indexCacheSize))
+ .weigher((Weigher<String, ShuffleIndexInformation>)
+ (filePath, indexInfo) -> indexInfo.getRetainedMemorySize());
+ int expireTimeSeconds = conf.shuffleIndexCacheExpireTimeSeconds();
+ if (expireTimeSeconds > 0) {
+ shuffleIndexCache = cacheBuilder.expireAfterAccess(expireTimeSeconds, TimeUnit.SECONDS)
Review Comment:
Using the timeout threshold doesn't seem to be a friendly config for users. I don't think users could think of a reasonable timeout value to clean up the cache. In the worst case, they could result in regression if they set an arbitrary value, e.g., a very small value.
The only case that I think we can remove the shuffle cache now is when `applicationRemoved` is called. In that case, I think we could remove the shuffle cache safely, which could mitigate the issue you mentioned. However, I didn't see guava cache has such an API to remove unused cache entries.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] github-actions[bot] commented on pull request #36088: [SPARK-38805][SHUFFLE] Automatically remove an expired indexFilePath from the ESS shuffleIndexCache or the PBS indexCache to save memory.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #36088:
URL: https://github.com/apache/spark/pull/36088#issuecomment-1257089532
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] github-actions[bot] closed pull request #36088: [SPARK-38805][SHUFFLE] Automatically remove an expired indexFilePath from the ESS shuffleIndexCache or the PBS indexCache to save memory.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #36088: [SPARK-38805][SHUFFLE] Automatically remove an expired indexFilePath from the ESS shuffleIndexCache or the PBS indexCache to save memory.
URL: https://github.com/apache/spark/pull/36088
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] weixiuli commented on pull request #36088: [SPARK-38805][SHUFFLE] Automatically remove an expired indexFilePath from the ESS shuffleIndexCache or the PBS indexCache to save memory.
Posted by GitBox <gi...@apache.org>.
weixiuli commented on PR #36088:
URL: https://github.com/apache/spark/pull/36088#issuecomment-1096199260
> The size should be bounded by `spark.shuffle.service.index.cache.size`; with the cache evicting entries based on LRU. Did you try configuring that to control the memory footprint instead ?
It may not be a good ideat to control the memory footprint by the `spark.shuffle.service.index.cache.size` and `spark.shuffle.push.server.mergedIndexCacheSize`. Since if we make them very small it might NOT avoid open/close the index files, if we make them very large it might waste memory. With this pr, it can automatically remove an expired indexFilePath from them. @mridulm
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org