[GitHub] [spark] LuciferYang commented on pull request #36403: [SPARK-39063][CORE] Remove `finalize()` method and related codes from `LevelDB/RocksDBIterator`

[GitBox <gi...@apache.org>]

LuciferYang commented on PR #36403:
URL: https://github.com/apache/spark/pull/36403#issuecomment-1114466277

   https://github.com/apache/spark/blob/5630f700768432396a948376f5b46b00d4186e1b/common/kvstore/src/main/java/org/apache/spark/util/kvstore/RocksDB.java#L332-L356
   
   Actually, if use a `rocksdbjni` **without jemalloc**, the above code has the risk of **jvm crash** due to `finalize()`.(this issue can be reproduced by `testCloseRocksDBIterator` in `RocksDBSuite` ).
   
   `synchronized (this._db)`  blocks the execution of `Finalization`, this will cause some `LevelDB/RocksDBIteraoter` not to be closed before `db` closed,  so `finalize()` does not really solve the problem(It seems that `jemalloc` avoided this problem). This issue maybe solved by calling `System.runFinalization()` before `synchronized (this._db)`. 
   
   On the other hand , `finalize()` is an API that JDK officials [no longer recommend](https://openjdk.java.net/jeps/421):
   
   https://github.com/openjdk/jdk/blob/16a8ebbf0573b8ee75072f8120fb0d4a584cb51d/src/java.base/share/classes/java/lang/Object.java#L552-L576
   
   Of course, I also agree to keep it until JDK really deletes 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