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/08/05 05:06:39 UTC
[GitHub] [spark] LuciferYang opened a new pull request, #37420: [SPARK-39988][CORE][YARN] Add `try-with-resource` to ensure `DBIterator` is close after use in `RemoteBlockPushResolver`, `YarnShuffleService` and `ExternalShuffleBlockResolver`
LuciferYang opened a new pull request, #37420:
URL: https://github.com/apache/spark/pull/37420
### What changes were proposed in this pull request?
This pr add `try-with-resource` to ensure `DBIterator` is close after use in `RemoteBlockPushResolver`, `YarnShuffleService` and `ExternalShuffleBlockResolver` to avoid resource leakage.
### Why are the changes needed?
Avoid `DBIterator` resource leakage.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Pass GitHub Actions
--
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] dongjoon-hyun commented on pull request #37420: [SPARK-39988][CORE] Use `try-with-resource` to ensure `DBIterator` is close after use
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #37420:
URL: https://github.com/apache/spark/pull/37420#issuecomment-1207691289
No, I don't think so for now, @LuciferYang . Let's keep this in master branch for Apache Spark 3.4.0 first.
--
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] LuciferYang commented on pull request #37420: [SPARK-39988][CORE] Use `try-with-resource` to ensure `DBIterator` is close after use in `RemoteBlockPushResolver`, `YarnShuffleService` and `ExternalShuffleBlockResolver`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37420:
URL: https://github.com/apache/spark/pull/37420#issuecomment-1206056318
cc @dongjoon-hyun @mridulm Changes of this pr is for bug fix, which is not within the refactor scope of SPARK-38909
--
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] LuciferYang commented on pull request #37420: [SPARK-39988][CORE] Use `try-with-resource` to ensure `DBIterator` is close after use
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37420:
URL: https://github.com/apache/spark/pull/37420#issuecomment-1207691608
OK
--
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] LuciferYang commented on a diff in pull request #37420: [SPARK-39988][CORE] Use `try-with-resource` to ensure `DBIterator` is close after use in `RemoteBlockPushResolver`, `YarnShuffleService` and `ExternalShuffleBlockResolver`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37420:
URL: https://github.com/apache/spark/pull/37420#discussion_r938449470
##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolver.java:
##########
@@ -457,18 +457,19 @@ static ConcurrentMap<AppExecId, ExecutorShuffleInfo> reloadRegisteredExecutors(D
throws IOException {
ConcurrentMap<AppExecId, ExecutorShuffleInfo> registeredExecutors = Maps.newConcurrentMap();
if (db != null) {
- DBIterator itr = db.iterator();
Review Comment:
The methods `reloadActiveAppAttemptsPathInfo` and `reloadFinalizedAppAttemptsShuffleMergeInfo` should be Spark 3.4 only. Please let me know if this PR needs to be backport to other branch
--
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] dongjoon-hyun closed pull request #37420: [SPARK-39988][CORE] Use `try-with-resource` to ensure `DBIterator` is close after use
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun closed pull request #37420: [SPARK-39988][CORE] Use `try-with-resource` to ensure `DBIterator` is close after use
URL: https://github.com/apache/spark/pull/37420
--
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] dongjoon-hyun commented on pull request #37420: [SPARK-39988][CORE] Use `try-with-resource` to ensure `DBIterator` is close after use
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #37420:
URL: https://github.com/apache/spark/pull/37420#issuecomment-1207575962
Merged to master.
--
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] LuciferYang commented on a diff in pull request #37420: [SPARK-39988][CORE] Use `try-with-resource` to ensure `DBIterator` is close after use in `RemoteBlockPushResolver`, `YarnShuffleService` and `ExternalShuffleBlockResolver`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37420:
URL: https://github.com/apache/spark/pull/37420#discussion_r938449470
##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolver.java:
##########
@@ -457,18 +457,19 @@ static ConcurrentMap<AppExecId, ExecutorShuffleInfo> reloadRegisteredExecutors(D
throws IOException {
ConcurrentMap<AppExecId, ExecutorShuffleInfo> registeredExecutors = Maps.newConcurrentMap();
if (db != null) {
- DBIterator itr = db.iterator();
Review Comment:
`reloadActiveAppAttemptsPathInfo` and `reloadFinalizedAppAttemptsShuffleMergeInfo` should be Spark 3.4 only. Please let me know if this PR needs to be backport to other branch
--
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] LuciferYang commented on pull request #37420: [SPARK-39988][CORE] Use `try-with-resource` to ensure `DBIterator` is close after use
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37420:
URL: https://github.com/apache/spark/pull/37420#issuecomment-1207582167
> Looks reasonable to me, but we had better ping more people in this area, @LuciferYang .
Sorry for not reply at the weekend, this issue exists in the historical version, do we need a backup port?
--
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