HeartSaVioR commented on pull request #28769: URL: https://github.com/apache/spark/pull/28769#issuecomment-641649819
I guess the interface matters - in-memory KV store doesn't need to have close in its iterator of course, but level DB KV store should. The code picked "Iterator" as common denominator which doesn't have "close" on it, so the caller has no idea whether it should call close or not, and once it's wrapped by others like asScala or so, the caller will never be able to call close afterwards. The implementation of iterator implements Closeable, but the caller is on Scala - can't rely on Java language's try-with-resource. I don't know the interface is public - I don't see any usage of KV store "outside" Spark or any plug-in interface to provide custom implementation of KV store "into" Spark - if it's private it'd be better to return KVStoreIterator, with making sure callers must call close. ---------------------------------------------------------------- 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]
