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/18 02:39:55 UTC
[GitHub] [spark] LuciferYang opened a new pull request, #36237: [SPARK-38896][CORE][SQL] Manual close `KVStoreIterator` use `tryWithResource`
LuciferYang opened a new pull request, #36237:
URL: https://github.com/apache/spark/pull/36237
### What changes were proposed in this pull request?
Similar to SPARK-38847, this pr aims to accelerate the recycling of `KVStoreIterator` resources by using `tryWithResource`. The main change of this pr as follows:
1. Use Java `tryWithResource` and Spark `Utils.tryWithResource` to recycling `KVStoreIterator` opened by `RocksDB.view(Class<T> type).iterator` and `RocksDB.view(Class<T> type).iterator`
2. Introduce a `count` function for KVUtils, this function will counts the number of elements in the KVStoreView which satisfy a predicate and close `KVStoreIterator` in time.
### Why are the changes needed?
Close `KVStoreIterator` in time.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Pass GA
--
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 #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #36237:
URL: https://github.com/apache/spark/pull/36237#issuecomment-1109227201
I close another PR, and I hope to fix similar cases all in one
--
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] srowen commented on a diff in pull request #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
srowen commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r852551619
##########
core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala:
##########
@@ -325,12 +325,18 @@ private[history] class FsHistoryProvider(conf: SparkConf, clock: Clock)
override def getListing(): Iterator[ApplicationInfo] = {
// Return the listing in end time descending order.
- listing.view(classOf[ApplicationInfoWrapper])
+ // SPARK-38896: tryWithResource cannot be used here
+ // because this method needs to return an `Iterator`.
+ val closeableIter = listing.view(classOf[ApplicationInfoWrapper])
.index("endTime")
.reverse()
- .iterator()
- .asScala
- .map(_.toApplicationInfo())
+ .closeableIterator()
+ val dataIter = closeableIter.asScala.map(_.toApplicationInfo())
+ new Iterator[ApplicationInfo] with Closeable {
+ override def hasNext: Boolean = dataIter.hasNext
+ override def next(): ApplicationInfo = dataIter.next()
+ override def close(): Unit = closeableIter.close()
+ }
Review Comment:
finalize() isn't wrong per se; relying on it can be. Given the context here, I think you would want to include it.
--
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 a diff in pull request #36237: [SPARK-38896][CORE][SQL] Manual close `KVStoreIterator` use `tryWithResource`
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r852355201
##########
core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala:
##########
@@ -325,12 +325,18 @@ private[history] class FsHistoryProvider(conf: SparkConf, clock: Clock)
override def getListing(): Iterator[ApplicationInfo] = {
// Return the listing in end time descending order.
- listing.view(classOf[ApplicationInfoWrapper])
+ // SPARK-38896: tryWithResource cannot be used here
+ // because this method needs to return an `Iterator`.
+ val closeableIter = listing.view(classOf[ApplicationInfoWrapper])
.index("endTime")
.reverse()
- .iterator()
- .asScala
- .map(_.toApplicationInfo())
+ .closeableIterator()
+ val dataIter = closeableIter.asScala.map(_.toApplicationInfo())
+ new Iterator[ApplicationInfo] with Closeable {
+ override def hasNext: Boolean = dataIter.hasNext
+ override def next(): ApplicationInfo = dataIter.next()
+ override def close(): Unit = closeableIter.close()
+ }
Review Comment:
Since not all usages of `getListing()` are draining the iterator, add a `finalize` as well ?
--
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] srowen commented on pull request #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
srowen commented on PR #36237:
URL: https://github.com/apache/spark/pull/36237#issuecomment-1111016305
Sorry I am still not clear. Do you want to review and merge this PR, now?
--
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] srowen closed pull request #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
srowen closed pull request #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
URL: https://github.com/apache/spark/pull/36237
--
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 #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #36237:
URL: https://github.com/apache/spark/pull/36237#issuecomment-1111026682
Yes， I want to review and merge this pr
--
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 #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #36237:
URL: https://github.com/apache/spark/pull/36237#issuecomment-1111685424
> Was there anything you wanted to change w.r.t. comment https://github.com/apache/spark/pull/36237/files#r852703387 ? Looking pretty fine
[f91bd0d](https://github.com/apache/spark/pull/36237/commits/f91bd0d3cf4262095f490996a698393e4de05d2b) fix this
--
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 #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r857059687
##########
core/src/main/scala/org/apache/spark/status/KVUtils.scala:
##########
@@ -100,6 +100,33 @@ private[spark] object KVUtils extends Logging {
}
}
+ /** Counts the number of elements in the KVStoreView which satisfy a predicate. */
+ def count[T](view: KVStoreView[T])(countFunc: T => Boolean): Int = {
Review Comment:
@srowen I separate the code related to these 4 new functions into a new pr: https://github.com/apache/spark/pull/36237/files
--
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 #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r853779042
##########
core/src/main/scala/org/apache/spark/status/KVUtils.scala:
##########
@@ -100,6 +100,33 @@ private[spark] object KVUtils extends Logging {
}
}
+ /** Counts the number of elements in the KVStoreView which satisfy a predicate. */
+ def count[T](view: KVStoreView[T])(countFunc: T => Boolean): Int = {
Review Comment:
@srowen Do you mind if I split the newly introduced functions and related refactoring work into another pr? It seems that the change of this part does not increase the complexity of the code.
--
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 #36237: [SPARK-38896][CORE][SQL] Manual close `KVStoreIterator` use `tryWithResource`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #36237:
URL: https://github.com/apache/spark/pull/36237#issuecomment-1101038365
Test link : https://github.com/LuciferYang/spark/runs/6051872250
--
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 #36237: [SPARK-38896][CORE][SQL] Manual close `KVStoreIterator` use `tryWithResource`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r851850646
##########
common/kvstore/src/test/java/org/apache/spark/util/kvstore/LevelDBBenchmark.java:
##########
@@ -197,9 +197,15 @@ private void iterate(KVStoreView<?> view, String name) throws Exception {
}
}
- while (it.hasNext()) {
- try(Timer.Context ctx = iter.time()) {
- it.next();
+ try {
+ while (it.hasNext()) {
+ try (Timer.Context ctx = iter.time()) {
+ it.next();
+ }
+ }
+ } finally {
Review Comment:
The last `it` is not closed
--
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 #36237: [SPARK-38896][CORE][SQL] Manual close `KVStoreIterator` use `tryWithResource`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r852542476
##########
core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala:
##########
@@ -325,12 +325,18 @@ private[history] class FsHistoryProvider(conf: SparkConf, clock: Clock)
override def getListing(): Iterator[ApplicationInfo] = {
// Return the listing in end time descending order.
- listing.view(classOf[ApplicationInfoWrapper])
+ // SPARK-38896: tryWithResource cannot be used here
+ // because this method needs to return an `Iterator`.
+ val closeableIter = listing.view(classOf[ApplicationInfoWrapper])
.index("endTime")
.reverse()
- .iterator()
- .asScala
- .map(_.toApplicationInfo())
+ .closeableIterator()
+ val dataIter = closeableIter.asScala.map(_.toApplicationInfo())
+ new Iterator[ApplicationInfo] with Closeable {
+ override def hasNext: Boolean = dataIter.hasNext
+ override def next(): ApplicationInfo = dataIter.next()
+ override def close(): Unit = closeableIter.close()
+ }
Review Comment:
`finalize` or `finally`?
--
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] srowen commented on pull request #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
srowen commented on PR #36237:
URL: https://github.com/apache/spark/pull/36237#issuecomment-1108606346
OK, which change do you want to go in first? this PR has both changes right now, it appears
--
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 #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r860426105
##########
core/src/test/scala/org/apache/spark/deploy/history/HybridStoreSuite.scala:
##########
@@ -103,6 +104,13 @@ abstract class HybridStoreSuite extends SparkFunSuite with BeforeAndAfter with T
}
test("test basic iteration") {
+
+ def firstInView[T](view: KVStoreView[T]): T = {
Review Comment:
[70528b6](https://github.com/apache/spark/pull/36237/commits/70528b6d0af97636845196e4a764fdce6d41e2db) fix this
--
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] srowen commented on a diff in pull request #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
srowen commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r854066333
##########
core/src/main/scala/org/apache/spark/status/KVUtils.scala:
##########
@@ -100,6 +100,33 @@ private[spark] object KVUtils extends Logging {
}
}
+ /** Counts the number of elements in the KVStoreView which satisfy a predicate. */
+ def count[T](view: KVStoreView[T])(countFunc: T => Boolean): Int = {
Review Comment:
I think that's 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 pull request #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #36237:
URL: https://github.com/apache/spark/pull/36237#issuecomment-1101933448
> Right, I agree it's good practice, just wondering what we gain from it. For the in-memory case it does nothing. It looks like this releases some RocksDB state for example and that's good to do earlier.
Yes, this pr is to release `LevelDB/RocksDBIterator` earlier and not relate to `InMemoryIterator`
--
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 #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r852554229
##########
core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala:
##########
@@ -325,12 +325,18 @@ private[history] class FsHistoryProvider(conf: SparkConf, clock: Clock)
override def getListing(): Iterator[ApplicationInfo] = {
// Return the listing in end time descending order.
- listing.view(classOf[ApplicationInfoWrapper])
+ // SPARK-38896: tryWithResource cannot be used here
+ // because this method needs to return an `Iterator`.
+ val closeableIter = listing.view(classOf[ApplicationInfoWrapper])
.index("endTime")
.reverse()
- .iterator()
- .asScala
- .map(_.toApplicationInfo())
+ .closeableIterator()
+ val dataIter = closeableIter.asScala.map(_.toApplicationInfo())
+ new Iterator[ApplicationInfo] with Closeable {
+ override def hasNext: Boolean = dataIter.hasNext
+ override def next(): ApplicationInfo = dataIter.next()
+ override def close(): Unit = closeableIter.close()
+ }
Review Comment:
Thank you for your help @srowen
--
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 #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #36237:
URL: https://github.com/apache/spark/pull/36237#issuecomment-1101963694
All Test passed: https://github.com/LuciferYang/spark/runs/6071316292
--
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 #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r860455377
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -757,7 +767,7 @@ private[spark] class AppStatusStore(
executorLogs,
AppStatusUtils.schedulerDelay(taskDataOld),
AppStatusUtils.gettingResultTime(taskDataOld))
- }.toSeq
+ }.toList
Review Comment:
This change is to make `HistoryServerSuite` passed, `toSeq` causes `LevelDB/RocksDBIterator` close in advance, while 'toList' does not.
[de2862e](https://github.com/apache/spark/pull/36237/commits/de2862ec2fd1caec1c74b092510982b287795b58) try to fixed the issue in a different way
--
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] srowen commented on pull request #36237: [SPARK-38896][CORE][SQL] Manual close `KVStoreIterator` use `tryWithResource`
Posted by GitBox <gi...@apache.org>.
srowen commented on PR #36237:
URL: https://github.com/apache/spark/pull/36237#issuecomment-1101450198
Right, I agree it's good practice, just wondering what we gain from it. For the in-memory case it does nothing. It looks like this releases some RocksDB state for example and that's good to do earlier.
--
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 #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r852547098
##########
core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala:
##########
@@ -325,12 +325,18 @@ private[history] class FsHistoryProvider(conf: SparkConf, clock: Clock)
override def getListing(): Iterator[ApplicationInfo] = {
// Return the listing in end time descending order.
- listing.view(classOf[ApplicationInfoWrapper])
+ // SPARK-38896: tryWithResource cannot be used here
+ // because this method needs to return an `Iterator`.
+ val closeableIter = listing.view(classOf[ApplicationInfoWrapper])
.index("endTime")
.reverse()
- .iterator()
- .asScala
- .map(_.toApplicationInfo())
+ .closeableIterator()
+ val dataIter = closeableIter.asScala.map(_.toApplicationInfo())
+ new Iterator[ApplicationInfo] with Closeable {
+ override def hasNext: Boolean = dataIter.hasNext
+ override def next(): ApplicationInfo = dataIter.next()
+ override def close(): Unit = closeableIter.close()
+ }
Review Comment:
@mridulm
If `finalize`, there is no need to change here(should revert the change of this pr). But I don't think it should be used `finalize()` anymore. It's not a good practice, and it has been identified as forRemoval=true after [JEP 421](https://openjdk.java.net/jeps/421)
If use `finally` here to close `LevelDB/RocksDBIterator`, the caller will not be able to consume data.
--
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 #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r852547098
##########
core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala:
##########
@@ -325,12 +325,18 @@ private[history] class FsHistoryProvider(conf: SparkConf, clock: Clock)
override def getListing(): Iterator[ApplicationInfo] = {
// Return the listing in end time descending order.
- listing.view(classOf[ApplicationInfoWrapper])
+ // SPARK-38896: tryWithResource cannot be used here
+ // because this method needs to return an `Iterator`.
+ val closeableIter = listing.view(classOf[ApplicationInfoWrapper])
.index("endTime")
.reverse()
- .iterator()
- .asScala
- .map(_.toApplicationInfo())
+ .closeableIterator()
+ val dataIter = closeableIter.asScala.map(_.toApplicationInfo())
+ new Iterator[ApplicationInfo] with Closeable {
+ override def hasNext: Boolean = dataIter.hasNext
+ override def next(): ApplicationInfo = dataIter.next()
+ override def close(): Unit = closeableIter.close()
+ }
Review Comment:
@mridulm
If `finalize`, there is no need to change here. But I don't think it should be used `finalize()` anymore. It's not a good practice, and it has been identified as forRemoval=true after [JEP 421](https://openjdk.java.net/jeps/421)
If use `finally` here to close `LevelDB/RocksDBIterator`, the caller will not be able to consume data.
--
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 #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r854067067
##########
core/src/main/scala/org/apache/spark/status/KVUtils.scala:
##########
@@ -100,6 +100,33 @@ private[spark] object KVUtils extends Logging {
}
}
+ /** Counts the number of elements in the KVStoreView which satisfy a predicate. */
+ def count[T](view: KVStoreView[T])(countFunc: T => Boolean): Int = {
Review Comment:
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] srowen commented on a diff in pull request #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
srowen commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r859847820
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -757,7 +767,7 @@ private[spark] class AppStatusStore(
executorLogs,
AppStatusUtils.schedulerDelay(taskDataOld),
AppStatusUtils.gettingResultTime(taskDataOld))
- }.toSeq
+ }.toList
Review Comment:
Out of curiosity, why this change?
##########
core/src/test/scala/org/apache/spark/deploy/history/HybridStoreSuite.scala:
##########
@@ -103,6 +104,13 @@ abstract class HybridStoreSuite extends SparkFunSuite with BeforeAndAfter with T
}
test("test basic iteration") {
+
+ def firstInView[T](view: KVStoreView[T]): T = {
Review Comment:
Do you want to call this "head"?
--
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 #36237: [SPARK-38896][CORE][SQL] Manual close `KVStoreIterator` use `tryWithResource`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #36237:
URL: https://github.com/apache/spark/pull/36237#issuecomment-1101061638
@srowen Java 9 provides [java.lang.ref.Cleaner](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/ref/Cleaner.java) which can achieve the same purpose as `finalize`, but Spark still need to support Java 8 now.
Are there any other recommended alternatives?
--
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 closed pull request #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
LuciferYang closed pull request #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
URL: https://github.com/apache/spark/pull/36237
--
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 #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #36237:
URL: https://github.com/apache/spark/pull/36237#issuecomment-1107999200
> That seems like an overlapping change - which one of the two is ready for review/merge? I would have imagined it's this PR?
OK， let's continue this pr
--
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 #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r852703387
##########
core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala:
##########
@@ -325,12 +325,18 @@ private[history] class FsHistoryProvider(conf: SparkConf, clock: Clock)
override def getListing(): Iterator[ApplicationInfo] = {
// Return the listing in end time descending order.
- listing.view(classOf[ApplicationInfoWrapper])
+ // SPARK-38896: tryWithResource cannot be used here
+ // because this method needs to return an `Iterator`.
+ val closeableIter = listing.view(classOf[ApplicationInfoWrapper])
.index("endTime")
.reverse()
- .iterator()
- .asScala
- .map(_.toApplicationInfo())
+ .closeableIterator()
+ val dataIter = closeableIter.asScala.map(_.toApplicationInfo())
+ new Iterator[ApplicationInfo] with Closeable {
+ override def hasNext: Boolean = dataIter.hasNext
+ override def next(): ApplicationInfo = dataIter.next()
+ override def close(): Unit = closeableIter.close()
+ }
Review Comment:
Or we can change to return
```
KVUtils.mapToSeq(listing.view(classOf[ApplicationInfoWrapper])
.index("endTime")
.reverse())(_.toApplicationInfo()).iterator
```
This seems simpler, and the caller doesn't need to care about closing `LevelDB/RocksDBIterator`, but it may use more memory
--
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 #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r852703387
##########
core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala:
##########
@@ -325,12 +325,18 @@ private[history] class FsHistoryProvider(conf: SparkConf, clock: Clock)
override def getListing(): Iterator[ApplicationInfo] = {
// Return the listing in end time descending order.
- listing.view(classOf[ApplicationInfoWrapper])
+ // SPARK-38896: tryWithResource cannot be used here
+ // because this method needs to return an `Iterator`.
+ val closeableIter = listing.view(classOf[ApplicationInfoWrapper])
.index("endTime")
.reverse()
- .iterator()
- .asScala
- .map(_.toApplicationInfo())
+ .closeableIterator()
+ val dataIter = closeableIter.asScala.map(_.toApplicationInfo())
+ new Iterator[ApplicationInfo] with Closeable {
+ override def hasNext: Boolean = dataIter.hasNext
+ override def next(): ApplicationInfo = dataIter.next()
+ override def close(): Unit = closeableIter.close()
+ }
Review Comment:
Or we can return
```
KVUtils.mapToSeq(listing.view(classOf[ApplicationInfoWrapper])
.index("endTime")
.reverse())(_.toApplicationInfo()).iterator
```
This seems simpler, and the caller doesn't need to care about closing `LevelDB/RocksDBIterator`, but it may use more memory
--
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] srowen commented on pull request #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
srowen commented on PR #36237:
URL: https://github.com/apache/spark/pull/36237#issuecomment-1112756049
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 #36237: [SPARK-38896][CORE][SQL] Manual close `KVStoreIterator` use `tryWithResource`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r851851749
##########
core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala:
##########
@@ -325,12 +325,18 @@ private[history] class FsHistoryProvider(conf: SparkConf, clock: Clock)
override def getListing(): Iterator[ApplicationInfo] = {
// Return the listing in end time descending order.
- listing.view(classOf[ApplicationInfoWrapper])
+ // SPARK-38896: tryWithResource cannot be used here
+ // because this method needs to return an `Iterator`.
+ val closeableIter = listing.view(classOf[ApplicationInfoWrapper])
Review Comment:
`tryWithResource` cannot be used here because this method needs to return an `Iterator`, mixed in `java.io.Closeable` so that the caller can close it
--
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 #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r860426589
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -757,7 +767,7 @@ private[spark] class AppStatusStore(
executorLogs,
AppStatusUtils.schedulerDelay(taskDataOld),
AppStatusUtils.gettingResultTime(taskDataOld))
- }.toSeq
+ }.toList
Review Comment:
Let me double check this
--
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 a diff in pull request #36237: [SPARK-38896][CORE][SQL] Manual close `KVStoreIterator` use `tryWithResource`
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r852355201
##########
core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala:
##########
@@ -325,12 +325,18 @@ private[history] class FsHistoryProvider(conf: SparkConf, clock: Clock)
override def getListing(): Iterator[ApplicationInfo] = {
// Return the listing in end time descending order.
- listing.view(classOf[ApplicationInfoWrapper])
+ // SPARK-38896: tryWithResource cannot be used here
+ // because this method needs to return an `Iterator`.
+ val closeableIter = listing.view(classOf[ApplicationInfoWrapper])
.index("endTime")
.reverse()
- .iterator()
- .asScala
- .map(_.toApplicationInfo())
+ .closeableIterator()
+ val dataIter = closeableIter.asScala.map(_.toApplicationInfo())
+ new Iterator[ApplicationInfo] with Closeable {
+ override def hasNext: Boolean = dataIter.hasNext
+ override def next(): ApplicationInfo = dataIter.next()
+ override def close(): Unit = closeableIter.close()
+ }
Review Comment:
Since not all usages of `getListing()` are draining the iterator, add a `finalize` as well ?
Alternative is to generalize and ensure all iterators are closed within history server (which are closeable).
--
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 #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r852547098
##########
core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala:
##########
@@ -325,12 +325,18 @@ private[history] class FsHistoryProvider(conf: SparkConf, clock: Clock)
override def getListing(): Iterator[ApplicationInfo] = {
// Return the listing in end time descending order.
- listing.view(classOf[ApplicationInfoWrapper])
+ // SPARK-38896: tryWithResource cannot be used here
+ // because this method needs to return an `Iterator`.
+ val closeableIter = listing.view(classOf[ApplicationInfoWrapper])
.index("endTime")
.reverse()
- .iterator()
- .asScala
- .map(_.toApplicationInfo())
+ .closeableIterator()
+ val dataIter = closeableIter.asScala.map(_.toApplicationInfo())
+ new Iterator[ApplicationInfo] with Closeable {
+ override def hasNext: Boolean = dataIter.hasNext
+ override def next(): ApplicationInfo = dataIter.next()
+ override def close(): Unit = closeableIter.close()
+ }
Review Comment:
If `finalize`, there is no need to change here. But I don't think it should be used `finalize()` anymore. It's not a good practice, and it has been identified as forRemoval=true after [JEP 421](https://openjdk.java.net/jeps/421)
If use `finally` here to close `LevelDB/RocksDBIterator`, the caller will not be able to consume data.
--
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 #36237: [SPARK-38896][CORE][SQL] Manual close `KVStoreIterator` use `tryWithResource`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #36237:
URL: https://github.com/apache/spark/pull/36237#issuecomment-1101049714
> I'm not against this, but it adds complexity. Do you have any rough sense of how much it improves things?
Sorry, I want to clarify the main goal of this pr: it is not for the benefit of performance, but to close `KVStoreIterator ` resources opened by `KVStore.view.iterator` in time.
I hope `finalize()` method can be cleaned up later after this pr because using `finalize` to recycle resources is not a good way( for example, Effective Java Item 7 Avoid finalizers) and `finalize()` is identified as `deprecated` after Java 11 and `forRemoval=true` after Java 18
--
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 #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #36237:
URL: https://github.com/apache/spark/pull/36237#issuecomment-1108007983
I am ok to continue on this pr
--
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] srowen commented on pull request #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
srowen commented on PR #36237:
URL: https://github.com/apache/spark/pull/36237#issuecomment-1107869213
That seems like an overlapping change - which one of the two is ready for review/merge? I would have imagined it's this PR?
--
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 #36237: [SPARK-38896][CORE][SQL] Manual close `KVStoreIterator` use `tryWithResource`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r851850154
##########
common/kvstore/src/test/java/org/apache/spark/util/kvstore/RocksDBSuite.java:
##########
@@ -255,12 +257,15 @@ public void testNegativeIndexValues() throws Exception {
}
});
- List<Integer> results = StreamSupport
- .stream(db.view(CustomType1.class).index("int").spliterator(), false)
- .map(e -> e.num)
- .collect(Collectors.toList());
+ try (KVStoreIterator<CustomType1> iterator =
+ db.view(CustomType1.class).index("int").closeableIterator()) {
+ List<Integer> results = StreamSupport
+ .stream(Spliterators.spliteratorUnknownSize(iterator, 0), false)
+ .map(e -> e.num)
+ .collect(Collectors.toList());
- assertEquals(expected, results);
+ assertEquals(expected, results);
+ }
}
@Test
Review Comment:
This pr clears all similar cases except `testCloseRocksDBIterator` in `LevelDB/RocksDBSuite` because this case seems use to check `finalize` mechanism.
After this pr, we can consider cleaning up `finalize()` related code from `LevelDB/RocksDB` because the `finalize` mechanism is not recommended, and it has been identified as `forRemoval=true` after [Java 18](https://openjdk.java.net/jeps/421)
cc @srowen @HyukjinKwon @dongjoon-hyun
--
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 #36237: [SPARK-38896][CORE][SQL] Manual close `KVStoreIterator` use `tryWithResource`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r851850771
##########
common/kvstore/src/test/java/org/apache/spark/util/kvstore/RocksDBBenchmark.java:
##########
@@ -196,10 +196,15 @@ private void iterate(KVStoreView<?> view, String name) throws Exception {
}
}
}
-
- while (it.hasNext()) {
- try(Timer.Context ctx = iter.time()) {
- it.next();
+ try {
+ while (it.hasNext()) {
+ try (Timer.Context ctx = iter.time()) {
+ it.next();
+ }
+ }
+ } finally {
Review Comment:
The last it is not closed
--
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 #36237: [SPARK-38896][CORE][SQL] Manual close `KVStoreIterator` use `tryWithResource`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #36237:
URL: https://github.com/apache/spark/pull/36237#issuecomment-1101051746
Updated 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.
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 #36237: [SPARK-38896][CORE][SQL] Use `tryWithResource` release `LevelDB/RocksDBIterator` resources earlier
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36237:
URL: https://github.com/apache/spark/pull/36237#discussion_r852547098
##########
core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala:
##########
@@ -325,12 +325,18 @@ private[history] class FsHistoryProvider(conf: SparkConf, clock: Clock)
override def getListing(): Iterator[ApplicationInfo] = {
// Return the listing in end time descending order.
- listing.view(classOf[ApplicationInfoWrapper])
+ // SPARK-38896: tryWithResource cannot be used here
+ // because this method needs to return an `Iterator`.
+ val closeableIter = listing.view(classOf[ApplicationInfoWrapper])
.index("endTime")
.reverse()
- .iterator()
- .asScala
- .map(_.toApplicationInfo())
+ .closeableIterator()
+ val dataIter = closeableIter.asScala.map(_.toApplicationInfo())
+ new Iterator[ApplicationInfo] with Closeable {
+ override def hasNext: Boolean = dataIter.hasNext
+ override def next(): ApplicationInfo = dataIter.next()
+ override def close(): Unit = closeableIter.close()
+ }
Review Comment:
@mridulm
If `finalize`, there is no need to change here(should revert the change of this pr). But I don't think it should be used `finalize()` anymore. It's not a good practice, and it has been identified as `forRemoval=true` after [JEP 421](https://openjdk.java.net/jeps/421)
If use `finally` here to close `LevelDB/RocksDBIterator`, the caller will not be able to consume data.
--
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