LuciferYang commented on code in PR #43502:
URL: https://github.com/apache/spark/pull/43502#discussion_r1373003887
##########
common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDBIterator.java:
##########
@@ -280,4 +285,23 @@ static int compare(byte[] a, byte[] b) {
return a.length - b.length;
}
+ private record ResourceCleaner(DBIterator dbIterator, LevelDB levelDB)
implements Runnable {
+ @Override
+ public void run() {
+ levelDB.getIteratorTracker().removeIf(ref -> {
+ LevelDBIterator<?> levelDBIterator = ref.get();
+ return levelDBIterator != null &&
dbIterator.equals(levelDBIterator.it);
Review Comment:
> looks like there is a leak in the DB, iterator's weak ref is never cleaned
up from the tracker ?
I think this should be a old problem, and we have also performed null checks
in the close method of RocksDB.
https://github.com/apache/spark/blob/7d7afb06f682c10f3900eb8adeab9fad6d49cb24/common/kvstore/src/main/java/org/apache/spark/util/kvstore/RocksDB.java#L341-L348
https://github.com/apache/spark/blob/7d7afb06f682c10f3900eb8adeab9fad6d49cb24/common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java#L307-L315
If an iterator is no longer referenced and is not manually closed, then if
GC occurs at this time, the iteratorTracker may still hold the Ref, but Ref#get
is null.
Should we change this condition to
```
return levelDBIterator == null || dbIterator == levelDBIterator.it;
```
Can this also clean up the invalid weakref in passing?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
