HeartSaVioR commented on a change in pull request #28769:
URL: https://github.com/apache/spark/pull/28769#discussion_r439782275
##########
File path:
common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java
##########
@@ -229,6 +241,10 @@ public long count(Class<?> type, String index, Object
indexedValue) throws Excep
return idx.getCount(idx.end(null, indexedValue));
}
+ /**
+ * Trying to close a JNI LevelDB handle with a closed DB can cause JVM
crashes,
Review comment:
nit: Trying to close a JNI LevelDB handle with a closed DB can cause JVM
crashes`. T`his ensures that all iterators are correctly closed before DB is
closed.
Btw, I guess this comment looks better than the one for `iteratorTracker`.
We may just need to have one in `iteratorTracker` with replacing the sentences,
and maybe better to add explanation of why we use soft reference to track these
iterators.
##########
File path:
common/kvstore/src/test/java/org/apache/spark/util/kvstore/LevelDBSuite.java
##########
@@ -276,6 +277,41 @@ public void testNegativeIndexValues() throws Exception {
assertEquals(expected, results);
}
+ @Test
+ public void testCloseLevelDBIterator() throws Exception {
+ // SPARK-31929: test when LevelDB.close() is called, related
LevelDBIterators
+ // are closed. And files opened by iterators are also closed.
+ File dbPathForCloseTest = File
+ .createTempFile(
+ "test_db_close.",
+ ".ldb");
+ dbPathForCloseTest.delete();
+ LevelDB dbForCloseTest = new LevelDB(dbPathForCloseTest);
+ for (int i = 0; i < 8192; i++) {
+ dbForCloseTest.write(createCustomType1(i));
+ }
+ String key = dbForCloseTest
+ .view(CustomType1.class).iterator().next().key;
+ assertEquals("key0", key);
+ Iterator<CustomType1> it0 = dbForCloseTest
+ .view(CustomType1.class).max(1).iterator();
+ while(it0.hasNext()) {
+ it0.next();
+ }
+ System.gc();
+ Iterator<CustomType1> it1 = dbForCloseTest
+ .view(CustomType1.class).iterator();
+ assertEquals("key0", it1.next().key);
+ try(KVStoreIterator<CustomType1> it2 = dbForCloseTest
Review comment:
nit: space after `try`
##########
File path:
common/kvstore/src/test/java/org/apache/spark/util/kvstore/LevelDBSuite.java
##########
@@ -276,6 +277,41 @@ public void testNegativeIndexValues() throws Exception {
assertEquals(expected, results);
}
+ @Test
+ public void testCloseLevelDBIterator() throws Exception {
Review comment:
I haven't tested but it might not fail on Linux/MacOS even without the
patch. That's OK, if you've tested with Windows and make sure this test fails
without the patch. Could you please confirm? (I don't have Windows env. for
development.)
----------------------------------------------------------------
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]
