HeartSaVioR edited a comment on pull request #28769: URL: https://github.com/apache/spark/pull/28769#issuecomment-641676010
> Lots of the paths actually do close the iterator explicitly. And I'm still not sure how .toSeq doesn't consume it. I'm not sure you're referring KV store iterator, as I don't see it. Could you please point out some places? > Recall that the underlying iterator closes itself when it consumes all elements. Maybe you refer to `CompletionIterator` which still need to call `close` explicitly on its implementation. That implementation is on core module and KV store module doesn't even know about such existence. > We can't get at the iterator that .asScala produces but we might be able to add a close() method to the view or something, but that's getting ugly I agree current code as pretty much concise, but I'm really worrying that we don't consider resource leak as serious one and be the one of items in trade-offs. I'm not sure I can agree with the view that resource leak can be tolerated to make code beauty. The problem wasn't observed because Linux deals with the file deletion on usage - if we have been having extensive Windows users then the problem should be raised much earlier. We can still play with try-with-resource pattern with the returned value of `closeableIterator` - it may still allow us to wrap with asScala, since it also implements Iterator. That forces us to live with a couple of more lines instead of one-liner but I'm not sure it matters. ---------------------------------------------------------------- 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]
