HeartSaVioR edited a comment on pull request #28769: URL: https://github.com/apache/spark/pull/28769#issuecomment-641668567
IMHO, KVStoreView shouldn't implement `Iterable` directly - this leads callers to simply call `iterator` or wrap with `asScala` and completely forget about the resource close. Callers should call `closeableIterator` explicitly to "indicate" they get an iterator which should be closed at the end of usage. Just documenting the warning on the class doc is not enough and that is actually also another kind of culprit. (No caller respected the warning.) Calling `close` in `finalize` as well. Besides of fixing resource leaks, it might be still needed to have a custom logic on LevelDB specific iterator like this PR, as we suppose the multi-threaded usages. Worth noting that Level DB KV store is sensitive on error as it leverages JNI - JVM crash would happen instead of some exception being thrown. ---------------------------------------------------------------- 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: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
