HeartSaVioR commented on a change in pull request #28769:
URL: https://github.com/apache/spark/pull/28769#discussion_r438360149
##########
File path: core/src/main/scala/org/apache/spark/status/AppStatusStore.scala
##########
@@ -39,7 +39,13 @@ private[spark] class AppStatusStore(
def applicationInfo(): v1.ApplicationInfo = {
try {
// The ApplicationInfo may not be available when Spark is starting up.
- store.view(classOf[ApplicationInfoWrapper]).max(1).iterator().next().info
+ Utils.tryWithResource(
Review comment:
(OFF-TOPIC, as my comment is for further PR, the change itself looks OK.)
@srowen
This change shows the drawback if we fix the leak properly. I'm trying to
implement at least `Iterable` to the `KVStoreIterator` so that
`iterator().asScala` would work, but still require much more lines than before.
I also see what you meant by toSeq - some callers don't materialize full
list and still want to have stream, but then we can't apply the resource
cleanup there. I'd say we cannot give up resource cleanup even for that case -
looks like there's no choice but just materialize if it is not known to be a
huge stream, otherwise just pass KVStoreIterator as a return and let caller
deal with cleanup.
Does it make sense?
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
