This is an automated email from the ASF dual-hosted git repository. srowen pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/spark.git
The following commit(s) were added to refs/heads/master by this push: new 8439d8473dd [SPARK-40036][CORE] Fix some API semantics of `Level/RocksDBIterator` after `db/iterator` is closed 8439d8473dd is described below commit 8439d8473dd462c06d606f10ba8fbff744b492c7 Author: yangjie01 <yangji...@baidu.com> AuthorDate: Tue Aug 16 08:44:33 2022 -0500 [SPARK-40036][CORE] Fix some API semantics of `Level/RocksDBIterator` after `db/iterator` is closed ### What changes were proposed in this pull request? This PR do the following changes to ensure that the semantics of `Level/RocksDBIterator`'s API are correct after `db/iterator` is closed - Add `next = null` to `close()` method of `Level/RocksDBIterator` to ensure `hasNext()` always return `false` and `next()` always throw `NoSuchElementException` after `db/iterator` is closed - Add `closed` to `skip(n)` method of `Level/RocksDBIterator` to ensure it always return `false` instead of throw `AssertionError` after `db/iterator` is closed ### Why are the changes needed? Bug fix: fix API semantics of `Level/RocksDBIterator` after `db/iterator` is closed ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? - Pass GitHub Actions - Add new test case Closes #37471 from LuciferYang/lrdb-hasNext. Authored-by: yangjie01 <yangji...@baidu.com> Signed-off-by: Sean Owen <sro...@gmail.com> --- .../apache/spark/util/kvstore/LevelDBIterator.java | 3 + .../apache/spark/util/kvstore/RocksDBIterator.java | 3 + .../apache/spark/util/kvstore/LevelDBSuite.java | 78 ++++++++++++++++++++++ .../apache/spark/util/kvstore/RocksDBSuite.java | 78 ++++++++++++++++++++++ 4 files changed, 162 insertions(+) diff --git a/common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDBIterator.java b/common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDBIterator.java index e8fb4fac5ba..62f7768ea7f 100644 --- a/common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDBIterator.java +++ b/common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDBIterator.java @@ -159,6 +159,8 @@ class LevelDBIterator<T> implements KVStoreIterator<T> { @Override public boolean skip(long n) { + if (closed) return false; + long skipped = 0; while (skipped < n) { if (next != null) { @@ -189,6 +191,7 @@ class LevelDBIterator<T> implements KVStoreIterator<T> { if (!closed) { it.close(); closed = true; + next = null; } } diff --git a/common/kvstore/src/main/java/org/apache/spark/util/kvstore/RocksDBIterator.java b/common/kvstore/src/main/java/org/apache/spark/util/kvstore/RocksDBIterator.java index 1db47f4dad0..fce50a5fc22 100644 --- a/common/kvstore/src/main/java/org/apache/spark/util/kvstore/RocksDBIterator.java +++ b/common/kvstore/src/main/java/org/apache/spark/util/kvstore/RocksDBIterator.java @@ -150,6 +150,8 @@ class RocksDBIterator<T> implements KVStoreIterator<T> { @Override public boolean skip(long n) { + if(closed) return false; + long skipped = 0; while (skipped < n) { if (next != null) { @@ -183,6 +185,7 @@ class RocksDBIterator<T> implements KVStoreIterator<T> { if (!closed) { it.close(); closed = true; + next = null; } } diff --git a/common/kvstore/src/test/java/org/apache/spark/util/kvstore/LevelDBSuite.java b/common/kvstore/src/test/java/org/apache/spark/util/kvstore/LevelDBSuite.java index ef0ccd4a639..86f65e9be89 100644 --- a/common/kvstore/src/test/java/org/apache/spark/util/kvstore/LevelDBSuite.java +++ b/common/kvstore/src/test/java/org/apache/spark/util/kvstore/LevelDBSuite.java @@ -305,6 +305,84 @@ public class LevelDBSuite { assertTrue(!dbPathForCloseTest.exists()); } + @Test + public void testHasNextAfterIteratorClose() throws Exception { + db.write(createCustomType1(0)); + KVStoreIterator<CustomType1> iter = + db.view(CustomType1.class).closeableIterator(); + // iter should be true + assertTrue(iter.hasNext()); + // close iter + iter.close(); + // iter.hasNext should be false after iter close + assertFalse(iter.hasNext()); + } + + @Test + public void testHasNextAfterDBClose() throws Exception { + db.write(createCustomType1(0)); + KVStoreIterator<CustomType1> iter = + db.view(CustomType1.class).closeableIterator(); + // iter should be true + assertTrue(iter.hasNext()); + // close db + db.close(); + // iter.hasNext should be false after db close + assertFalse(iter.hasNext()); + } + + @Test + public void testNextAfterIteratorClose() throws Exception { + db.write(createCustomType1(0)); + KVStoreIterator<CustomType1> iter = + db.view(CustomType1.class).closeableIterator(); + // iter should be true + assertTrue(iter.hasNext()); + // close iter + iter.close(); + // iter.next should throw NoSuchElementException after iter close + assertThrows(NoSuchElementException.class, iter::next); + } + + @Test + public void testNextAfterDBClose() throws Exception { + db.write(createCustomType1(0)); + KVStoreIterator<CustomType1> iter = + db.view(CustomType1.class).closeableIterator(); + // iter should be true + assertTrue(iter.hasNext()); + // close db + iter.close(); + // iter.next should throw NoSuchElementException after db close + assertThrows(NoSuchElementException.class, iter::next); + } + + @Test + public void testSkipAfterIteratorClose() throws Exception { + db.write(createCustomType1(0)); + KVStoreIterator<CustomType1> iter = + db.view(CustomType1.class).closeableIterator(); + // close iter + iter.close(); + // skip should always return false after iter close + assertFalse(iter.skip(0)); + assertFalse(iter.skip(1)); + } + + @Test + public void testSkipAfterDBClose() throws Exception { + db.write(createCustomType1(0)); + KVStoreIterator<CustomType1> iter = + db.view(CustomType1.class).closeableIterator(); + // iter should be true + assertTrue(iter.hasNext()); + // close db + db.close(); + // skip should always return false after db close + assertFalse(iter.skip(0)); + assertFalse(iter.skip(1)); + } + private CustomType1 createCustomType1(int i) { CustomType1 t = new CustomType1(); t.key = "key" + i; diff --git a/common/kvstore/src/test/java/org/apache/spark/util/kvstore/RocksDBSuite.java b/common/kvstore/src/test/java/org/apache/spark/util/kvstore/RocksDBSuite.java index a3ac40efdfb..602ab2d6881 100644 --- a/common/kvstore/src/test/java/org/apache/spark/util/kvstore/RocksDBSuite.java +++ b/common/kvstore/src/test/java/org/apache/spark/util/kvstore/RocksDBSuite.java @@ -303,6 +303,84 @@ public class RocksDBSuite { assertTrue(!dbPathForCloseTest.exists()); } + @Test + public void testHasNextAfterIteratorClose() throws Exception { + db.write(createCustomType1(0)); + KVStoreIterator<CustomType1> iter = + db.view(CustomType1.class).closeableIterator(); + // iter should be true + assertTrue(iter.hasNext()); + // close iter + iter.close(); + // iter.hasNext should be false after iter close + assertFalse(iter.hasNext()); + } + + @Test + public void testHasNextAfterDBClose() throws Exception { + db.write(createCustomType1(0)); + KVStoreIterator<CustomType1> iter = + db.view(CustomType1.class).closeableIterator(); + // iter should be true + assertTrue(iter.hasNext()); + // close db + db.close(); + // iter.hasNext should be false after db close + assertFalse(iter.hasNext()); + } + + @Test + public void testNextAfterIteratorClose() throws Exception { + db.write(createCustomType1(0)); + KVStoreIterator<CustomType1> iter = + db.view(CustomType1.class).closeableIterator(); + // iter should be true + assertTrue(iter.hasNext()); + // close iter + iter.close(); + // iter.next should throw NoSuchElementException after iter close + assertThrows(NoSuchElementException.class, iter::next); + } + + @Test + public void testNextAfterDBClose() throws Exception { + db.write(createCustomType1(0)); + KVStoreIterator<CustomType1> iter = + db.view(CustomType1.class).closeableIterator(); + // iter should be true + assertTrue(iter.hasNext()); + // close db + iter.close(); + // iter.next should throw NoSuchElementException after db close + assertThrows(NoSuchElementException.class, iter::next); + } + + @Test + public void testSkipAfterIteratorClose() throws Exception { + db.write(createCustomType1(0)); + KVStoreIterator<CustomType1> iter = + db.view(CustomType1.class).closeableIterator(); + // close iter + iter.close(); + // skip should always return false after iter close + assertFalse(iter.skip(0)); + assertFalse(iter.skip(1)); + } + + @Test + public void testSkipAfterDBClose() throws Exception { + db.write(createCustomType1(0)); + KVStoreIterator<CustomType1> iter = + db.view(CustomType1.class).closeableIterator(); + // iter should be true + assertTrue(iter.hasNext()); + // close db + db.close(); + // skip should always return false after db close + assertFalse(iter.skip(0)); + assertFalse(iter.skip(1)); + } + private CustomType1 createCustomType1(int i) { CustomType1 t = new CustomType1(); t.key = "key" + i; --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@spark.apache.org For additional commands, e-mail: commits-h...@spark.apache.org