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
