From 80a885427ad3e9ffc4e84f3cbfcf2a02351e80a0 Mon Sep 17 00:00:00 2001 From: Xiduo You Date: Wed, 2 Sep 2026 13:42:14 +0800 Subject: [PATCH 1/4] [CORE] Avoid NoSuchElementException on missing keys in KVStore reads and writes RocksDB.get and LevelDB.get now return null instead of throwing NoSuchElementException for a missing key, letting callers decide whether a missing key is an error. updateBatch previously threw and filled in an exception stack trace on every write of a new entry, and most writes during an event log rebuild are new entries, so this removes that overhead. read keeps throwing NoSuchElementException to preserve the KVStore contract; getMetadata, the constructor and the secondary-index iterator path handle the null directly. Assisted-by: Claude Opus 4.8 --- .../apache/spark/util/kvstore/LevelDB.java | 34 ++++++++----------- .../spark/util/kvstore/LevelDBIterator.java | 3 ++ .../apache/spark/util/kvstore/RocksDB.java | 34 ++++++++----------- .../spark/util/kvstore/RocksDBIterator.java | 3 ++ .../spark/util/kvstore/LevelDBSuite.java | 14 ++++++++ .../spark/util/kvstore/RocksDBSuite.java | 14 ++++++++ 6 files changed, 64 insertions(+), 38 deletions(-) diff --git a/common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java b/common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java index 91b2cde2d84fe..327baa6914144 100644 --- a/common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java +++ b/common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java @@ -98,12 +98,8 @@ public LevelDB(File path, KVStoreSerializer serializer) throws Exception { db().put(STORE_VERSION_KEY, serializer.serialize(STORE_VERSION)); } - Map aliases; - try { - aliases = get(TYPE_ALIASES_KEY, TypeAliases.class).aliases; - } catch (NoSuchElementException e) { - aliases = new HashMap<>(); - } + TypeAliases aliasesValue = get(TYPE_ALIASES_KEY, TypeAliases.class); + Map aliases = aliasesValue != null ? aliasesValue.aliases : new HashMap<>(); typeAliases = new ConcurrentHashMap<>(aliases); iteratorTracker = new ConcurrentLinkedQueue<>(); @@ -111,11 +107,7 @@ public LevelDB(File path, KVStoreSerializer serializer) throws Exception { @Override public T getMetadata(Class klass) throws Exception { - try { - return get(METADATA_KEY, klass); - } catch (NoSuchElementException nsee) { - return null; - } + return get(METADATA_KEY, klass); } @Override @@ -127,10 +119,15 @@ public void setMetadata(Object value) throws Exception { } } + /** + * Returns the value for the given key, or {@code null} if the key is not present. Callers + * that need to signal missing keys must check for {@code null} themselves, so that a missing + * key does not pay the cost of throwing and filling in an exception stack trace. + */ T get(byte[] key, Class klass) throws Exception { byte[] data = db().get(key); if (data == null) { - throw new NoSuchElementException(new String(key, UTF_8)); + return null; } return serializer.deserialize(data, klass); } @@ -144,7 +141,11 @@ private void put(byte[] key, Object value) throws Exception { public T read(Class klass, Object naturalKey) throws Exception { JavaUtils.checkArgument(naturalKey != null, "Null keys are not allowed."); byte[] key = getTypeInfo(klass).naturalIndex().start(null, naturalKey); - return get(key, klass); + T value = get(key, klass); + if (value == null) { + throw new NoSuchElementException(new String(key, UTF_8)); + } + return value; } @Override @@ -207,12 +208,7 @@ private void updateBatch( Class klass, LevelDBTypeInfo.Index naturalIndex, Collection indices) throws Exception { - Object existing; - try { - existing = get(naturalIndex.entityKey(null, value), klass); - } catch (NoSuchElementException e) { - existing = null; - } + Object existing = get(naturalIndex.entityKey(null, value), klass); PrefixCache cache = new PrefixCache(value); byte[] naturalKey = naturalIndex.toKey(naturalIndex.getValue(value)); 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 03bd2a3f12485..0bfd72afa8406 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 @@ -147,6 +147,9 @@ public T next() { } else { byte[] key = ti.buildKey(false, ti.naturalIndex().keyPrefix(null), next); ret = db.get(key, type); + if (ret == null) { + throw new NoSuchElementException(); + } } next = null; return ret; diff --git a/common/kvstore/src/main/java/org/apache/spark/util/kvstore/RocksDB.java b/common/kvstore/src/main/java/org/apache/spark/util/kvstore/RocksDB.java index 4b69b9441dc32..04abfcb2ee762 100644 --- a/common/kvstore/src/main/java/org/apache/spark/util/kvstore/RocksDB.java +++ b/common/kvstore/src/main/java/org/apache/spark/util/kvstore/RocksDB.java @@ -131,12 +131,8 @@ public RocksDB(File path, KVStoreSerializer serializer) throws Exception { db().put(STORE_VERSION_KEY, serializer.serialize(STORE_VERSION)); } - Map aliases; - try { - aliases = get(TYPE_ALIASES_KEY, TypeAliases.class).aliases; - } catch (NoSuchElementException e) { - aliases = new HashMap<>(); - } + TypeAliases aliasesValue = get(TYPE_ALIASES_KEY, TypeAliases.class); + Map aliases = aliasesValue != null ? aliasesValue.aliases : new HashMap<>(); typeAliases = new ConcurrentHashMap<>(aliases); iteratorTracker = new ConcurrentLinkedQueue<>(); @@ -144,11 +140,7 @@ public RocksDB(File path, KVStoreSerializer serializer) throws Exception { @Override public T getMetadata(Class klass) throws Exception { - try { - return get(METADATA_KEY, klass); - } catch (NoSuchElementException nsee) { - return null; - } + return get(METADATA_KEY, klass); } @Override @@ -160,10 +152,15 @@ public void setMetadata(Object value) throws Exception { } } + /** + * Returns the value for the given key, or {@code null} if the key is not present. Callers + * that need to signal missing keys must check for {@code null} themselves, so that a missing + * key does not pay the cost of throwing and filling in an exception stack trace. + */ T get(byte[] key, Class klass) throws Exception { byte[] data = db().get(key); if (data == null) { - throw new NoSuchElementException(new String(key, UTF_8)); + return null; } return serializer.deserialize(data, klass); } @@ -177,7 +174,11 @@ private void put(byte[] key, Object value) throws Exception { public T read(Class klass, Object naturalKey) throws Exception { JavaUtils.checkArgument(naturalKey != null, "Null keys are not allowed."); byte[] key = getTypeInfo(klass).naturalIndex().start(null, naturalKey); - return get(key, klass); + T value = get(key, klass); + if (value == null) { + throw new NoSuchElementException(new String(key, UTF_8)); + } + return value; } @Override @@ -239,12 +240,7 @@ private void updateBatch( Class klass, RocksDBTypeInfo.Index naturalIndex, Collection indices) throws Exception { - Object existing; - try { - existing = get(naturalIndex.entityKey(null, value), klass); - } catch (NoSuchElementException e) { - existing = null; - } + Object existing = get(naturalIndex.entityKey(null, value), klass); PrefixCache cache = new PrefixCache(value); byte[] naturalKey = naturalIndex.toKey(naturalIndex.getValue(value)); 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 a77f399a49c8b..99ef2f2095411 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 @@ -133,6 +133,9 @@ public T next() { } else { byte[] key = ti.buildKey(false, ti.naturalIndex().keyPrefix(null), next); ret = db.get(key, type); + if (ret == null) { + throw new NoSuchElementException(); + } } next = null; return ret; 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 25e6664d28dd1..76eefd8e268e9 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 @@ -98,6 +98,20 @@ public void testObjectWriteReadDelete() throws Exception { assertEquals(0, countKeys(t.getClass())); } + @Test + public void testGetMissingKeyReturnsNull() throws Exception { + // get() returns null instead of throwing for a missing key, so the write path does not + // pay the cost of building an exception when the entry does not exist yet; read() still + // surfaces a missing key as NoSuchElementException. + byte[] missingKey = db.getTypeInfo(CustomType1.class).naturalIndex().start(null, "missing"); + assertNull(db.get(missingKey, CustomType1.class)); + + CustomType1 t = createCustomType1(1); + db.write(t); + byte[] presentKey = db.getTypeInfo(CustomType1.class).naturalIndex().start(null, t.key); + assertEquals(t, db.get(presentKey, CustomType1.class)); + } + @Test public void testMultipleObjectWriteReadDelete() throws Exception { CustomType1 t1 = createCustomType1(1); 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 1334386fde74d..2096bb63b4cfe 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 @@ -95,6 +95,20 @@ public void testObjectWriteReadDelete() throws Exception { assertEquals(0, countKeys(t.getClass())); } + @Test + public void testGetMissingKeyReturnsNull() throws Exception { + // get() returns null instead of throwing for a missing key, so the write path does not + // pay the cost of building an exception when the entry does not exist yet; read() still + // surfaces a missing key as NoSuchElementException. + byte[] missingKey = db.getTypeInfo(CustomType1.class).naturalIndex().start(null, "missing"); + assertNull(db.get(missingKey, CustomType1.class)); + + CustomType1 t = createCustomType1(1); + db.write(t); + byte[] presentKey = db.getTypeInfo(CustomType1.class).naturalIndex().start(null, t.key); + assertEquals(t, db.get(presentKey, CustomType1.class)); + } + @Test public void testMultipleObjectWriteReadDelete() throws Exception { CustomType1 t1 = createCustomType1(1); From 9ba870b8c97acbeb778c03e530a84e763ed0f000 Mon Sep 17 00:00:00 2001 From: Xiduo You Date: Wed, 2 Sep 2026 21:22:02 +0800 Subject: [PATCH 2/4] [CORE] Carry missing key in KVStore iterator exceptions The secondary-index iterator paths resolve entries through a live get() and now include the missing key in the NoSuchElementException message, matching read(). Adds testNextAfterEntityDelete to both DB suites. Assisted-by: Claude Opus 4.8 --- .../spark/util/kvstore/LevelDBIterator.java | 3 ++- .../spark/util/kvstore/RocksDBIterator.java | 3 ++- .../spark/util/kvstore/LevelDBSuite.java | 18 ++++++++++++++++++ .../spark/util/kvstore/RocksDBSuite.java | 18 ++++++++++++++++++ 4 files changed, 40 insertions(+), 2 deletions(-) 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 0bfd72afa8406..1e80773cffa4f 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 @@ -25,6 +25,7 @@ import java.util.Map; import java.util.NoSuchElementException; import java.util.concurrent.atomic.AtomicBoolean; +import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.annotations.VisibleForTesting; import org.iq80.leveldb.DBIterator; @@ -148,7 +149,7 @@ public T next() { byte[] key = ti.buildKey(false, ti.naturalIndex().keyPrefix(null), next); ret = db.get(key, type); if (ret == null) { - throw new NoSuchElementException(); + throw new NoSuchElementException(new String(key, UTF_8)); } } 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 99ef2f2095411..c03567293ce46 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 @@ -21,6 +21,7 @@ import java.lang.ref.Cleaner; import java.util.*; import java.util.concurrent.atomic.AtomicBoolean; +import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.annotations.VisibleForTesting; import org.rocksdb.RocksIterator; @@ -134,7 +135,7 @@ public T next() { byte[] key = ti.buildKey(false, ti.naturalIndex().keyPrefix(null), next); ret = db.get(key, type); if (ret == null) { - throw new NoSuchElementException(); + throw new NoSuchElementException(new String(key, UTF_8)); } } 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 76eefd8e268e9..c194a68f11107 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 @@ -112,6 +112,24 @@ public void testGetMissingKeyReturnsNull() throws Exception { assertEquals(t, db.get(presentKey, CustomType1.class)); } + @Test + public void testNextAfterEntityDelete() throws Exception { + CustomType1 t = createCustomType1(1); + db.write(t); + + // Entries of a non-copy secondary index hold only the natural key, and next() resolves + // the entity with a live db.get() lookup. + try (KVStoreIterator it = + db.view(CustomType1.class).index("id").closeableIterator()) { + assertTrue(it.hasNext()); + // Delete the entity after hasNext() buffered its index entry: the iterator still + // sees the entry, but the live lookup in next() misses the key. + db.delete(t.getClass(), t.key); + NoSuchElementException e = assertThrows(NoSuchElementException.class, it::next); + assertTrue(e.getMessage().contains(t.key)); + } + } + @Test public void testMultipleObjectWriteReadDelete() throws Exception { CustomType1 t1 = createCustomType1(1); 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 2096bb63b4cfe..9d80c2fabd956 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 @@ -109,6 +109,24 @@ public void testGetMissingKeyReturnsNull() throws Exception { assertEquals(t, db.get(presentKey, CustomType1.class)); } + @Test + public void testNextAfterEntityDelete() throws Exception { + CustomType1 t = createCustomType1(1); + db.write(t); + + // Entries of a non-copy secondary index hold only the natural key, and next() resolves + // the entity with a live db.get() lookup. + try (KVStoreIterator it = + db.view(CustomType1.class).index("id").closeableIterator()) { + assertTrue(it.hasNext()); + // Delete the entity after hasNext() buffered its index entry: the iterator still + // sees the entry, but the live lookup in next() misses the key. + db.delete(t.getClass(), t.key); + NoSuchElementException e = assertThrows(NoSuchElementException.class, it::next); + assertTrue(e.getMessage().contains(t.key)); + } + } + @Test public void testMultipleObjectWriteReadDelete() throws Exception { CustomType1 t1 = createCustomType1(1); From 8acb98509cad567ef9f1e90af0d3978074e8ccb9 Mon Sep 17 00:00:00 2001 From: Xiduo You Date: Thu, 3 Sep 2026 09:56:26 +0800 Subject: [PATCH 3/4] [CORE] Rework KVStore missing-key handling around getOrNull Keep get() throwing NoSuchElementException and add a null-returning getOrNull() for callers where a missing key is expected: the store constructor, getMetadata and updateBatch. Also drop the catch blocks in delete() that nothing inside their try bodies can throw. Assisted-by: Claude Opus 4.8 --- .../apache/spark/util/kvstore/LevelDB.java | 36 ++++++++-------- .../spark/util/kvstore/LevelDBIterator.java | 4 -- .../apache/spark/util/kvstore/RocksDB.java | 36 ++++++++-------- .../spark/util/kvstore/RocksDBIterator.java | 4 -- .../spark/util/kvstore/LevelDBSuite.java | 41 +++++++++---------- .../spark/util/kvstore/RocksDBSuite.java | 41 +++++++++---------- 6 files changed, 76 insertions(+), 86 deletions(-) diff --git a/common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java b/common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java index 327baa6914144..daf86837031c5 100644 --- a/common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java +++ b/common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java @@ -98,7 +98,7 @@ public LevelDB(File path, KVStoreSerializer serializer) throws Exception { db().put(STORE_VERSION_KEY, serializer.serialize(STORE_VERSION)); } - TypeAliases aliasesValue = get(TYPE_ALIASES_KEY, TypeAliases.class); + TypeAliases aliasesValue = getOrNull(TYPE_ALIASES_KEY, TypeAliases.class); Map aliases = aliasesValue != null ? aliasesValue.aliases : new HashMap<>(); typeAliases = new ConcurrentHashMap<>(aliases); @@ -107,7 +107,7 @@ public LevelDB(File path, KVStoreSerializer serializer) throws Exception { @Override public T getMetadata(Class klass) throws Exception { - return get(METADATA_KEY, klass); + return getOrNull(METADATA_KEY, klass); } @Override @@ -119,17 +119,23 @@ public void setMetadata(Object value) throws Exception { } } + T get(byte[] key, Class klass) throws Exception { + T value = getOrNull(key, klass); + if (value == null) { + throw new NoSuchElementException(new String(key, UTF_8)); + } + return value; + } + /** - * Returns the value for the given key, or {@code null} if the key is not present. Callers - * that need to signal missing keys must check for {@code null} themselves, so that a missing - * key does not pay the cost of throwing and filling in an exception stack trace. + * Returns the value for the given key, or {@code null} if the key is not present, so that + * callers where a missing key is expected do not pay the cost of throwing and filling in an + * exception stack trace. */ - T get(byte[] key, Class klass) throws Exception { + @VisibleForTesting + T getOrNull(byte[] key, Class klass) throws Exception { byte[] data = db().get(key); - if (data == null) { - return null; - } - return serializer.deserialize(data, klass); + return data != null ? serializer.deserialize(data, klass) : null; } private void put(byte[] key, Object value) throws Exception { @@ -141,11 +147,7 @@ private void put(byte[] key, Object value) throws Exception { public T read(Class klass, Object naturalKey) throws Exception { JavaUtils.checkArgument(naturalKey != null, "Null keys are not allowed."); byte[] key = getTypeInfo(klass).naturalIndex().start(null, naturalKey); - T value = get(key, klass); - if (value == null) { - throw new NoSuchElementException(new String(key, UTF_8)); - } - return value; + return get(key, klass); } @Override @@ -208,7 +210,7 @@ private void updateBatch( Class klass, LevelDBTypeInfo.Index naturalIndex, Collection indices) throws Exception { - Object existing = get(naturalIndex.entityKey(null, value), klass); + Object existing = getOrNull(naturalIndex.entityKey(null, value), klass); PrefixCache cache = new PrefixCache(value); byte[] naturalKey = naturalIndex.toKey(naturalIndex.getValue(value)); @@ -236,8 +238,6 @@ public void delete(Class type, Object naturalKey) throws Exception { db().write(batch); } } - } catch (NoSuchElementException nse) { - // Ignore. } } 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 1e80773cffa4f..03bd2a3f12485 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 @@ -25,7 +25,6 @@ import java.util.Map; import java.util.NoSuchElementException; import java.util.concurrent.atomic.AtomicBoolean; -import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.annotations.VisibleForTesting; import org.iq80.leveldb.DBIterator; @@ -148,9 +147,6 @@ public T next() { } else { byte[] key = ti.buildKey(false, ti.naturalIndex().keyPrefix(null), next); ret = db.get(key, type); - if (ret == null) { - throw new NoSuchElementException(new String(key, UTF_8)); - } } next = null; return ret; diff --git a/common/kvstore/src/main/java/org/apache/spark/util/kvstore/RocksDB.java b/common/kvstore/src/main/java/org/apache/spark/util/kvstore/RocksDB.java index 04abfcb2ee762..fadb316bfd8a8 100644 --- a/common/kvstore/src/main/java/org/apache/spark/util/kvstore/RocksDB.java +++ b/common/kvstore/src/main/java/org/apache/spark/util/kvstore/RocksDB.java @@ -131,7 +131,7 @@ public RocksDB(File path, KVStoreSerializer serializer) throws Exception { db().put(STORE_VERSION_KEY, serializer.serialize(STORE_VERSION)); } - TypeAliases aliasesValue = get(TYPE_ALIASES_KEY, TypeAliases.class); + TypeAliases aliasesValue = getOrNull(TYPE_ALIASES_KEY, TypeAliases.class); Map aliases = aliasesValue != null ? aliasesValue.aliases : new HashMap<>(); typeAliases = new ConcurrentHashMap<>(aliases); @@ -140,7 +140,7 @@ public RocksDB(File path, KVStoreSerializer serializer) throws Exception { @Override public T getMetadata(Class klass) throws Exception { - return get(METADATA_KEY, klass); + return getOrNull(METADATA_KEY, klass); } @Override @@ -152,17 +152,23 @@ public void setMetadata(Object value) throws Exception { } } + T get(byte[] key, Class klass) throws Exception { + T value = getOrNull(key, klass); + if (value == null) { + throw new NoSuchElementException(new String(key, UTF_8)); + } + return value; + } + /** - * Returns the value for the given key, or {@code null} if the key is not present. Callers - * that need to signal missing keys must check for {@code null} themselves, so that a missing - * key does not pay the cost of throwing and filling in an exception stack trace. + * Returns the value for the given key, or {@code null} if the key is not present, so that + * callers where a missing key is expected do not pay the cost of throwing and filling in an + * exception stack trace. */ - T get(byte[] key, Class klass) throws Exception { + @VisibleForTesting + T getOrNull(byte[] key, Class klass) throws Exception { byte[] data = db().get(key); - if (data == null) { - return null; - } - return serializer.deserialize(data, klass); + return data != null ? serializer.deserialize(data, klass) : null; } private void put(byte[] key, Object value) throws Exception { @@ -174,11 +180,7 @@ private void put(byte[] key, Object value) throws Exception { public T read(Class klass, Object naturalKey) throws Exception { JavaUtils.checkArgument(naturalKey != null, "Null keys are not allowed."); byte[] key = getTypeInfo(klass).naturalIndex().start(null, naturalKey); - T value = get(key, klass); - if (value == null) { - throw new NoSuchElementException(new String(key, UTF_8)); - } - return value; + return get(key, klass); } @Override @@ -240,7 +242,7 @@ private void updateBatch( Class klass, RocksDBTypeInfo.Index naturalIndex, Collection indices) throws Exception { - Object existing = get(naturalIndex.entityKey(null, value), klass); + Object existing = getOrNull(naturalIndex.entityKey(null, value), klass); PrefixCache cache = new PrefixCache(value); byte[] naturalKey = naturalIndex.toKey(naturalIndex.getValue(value)); @@ -268,8 +270,6 @@ public void delete(Class type, Object naturalKey) throws Exception { db().write(writeOptions, writeBatch); } } - } catch (NoSuchElementException nse) { - // Ignore. } } 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 c03567293ce46..a77f399a49c8b 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 @@ -21,7 +21,6 @@ import java.lang.ref.Cleaner; import java.util.*; import java.util.concurrent.atomic.AtomicBoolean; -import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.annotations.VisibleForTesting; import org.rocksdb.RocksIterator; @@ -134,9 +133,6 @@ public T next() { } else { byte[] key = ti.buildKey(false, ti.naturalIndex().keyPrefix(null), next); ret = db.get(key, type); - if (ret == null) { - throw new NoSuchElementException(new String(key, UTF_8)); - } } next = null; return ret; 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 c194a68f11107..057bc7cd20b27 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 @@ -99,35 +99,34 @@ public void testObjectWriteReadDelete() throws Exception { } @Test - public void testGetMissingKeyReturnsNull() throws Exception { - // get() returns null instead of throwing for a missing key, so the write path does not - // pay the cost of building an exception when the entry does not exist yet; read() still - // surfaces a missing key as NoSuchElementException. + public void testGetOrNullMissingKey() throws Exception { + // getOrNull() returns null for a missing key so expected misses (e.g. the write path + // looking up an existing entry) skip the cost of building an exception, while get() + // still surfaces a missing key as NoSuchElementException. byte[] missingKey = db.getTypeInfo(CustomType1.class).naturalIndex().start(null, "missing"); - assertNull(db.get(missingKey, CustomType1.class)); + assertNull(db.getOrNull(missingKey, CustomType1.class)); + assertThrows(NoSuchElementException.class, () -> db.get(missingKey, CustomType1.class)); CustomType1 t = createCustomType1(1); db.write(t); byte[] presentKey = db.getTypeInfo(CustomType1.class).naturalIndex().start(null, t.key); - assertEquals(t, db.get(presentKey, CustomType1.class)); + assertEquals(t, db.getOrNull(presentKey, CustomType1.class)); } @Test - public void testNextAfterEntityDelete() throws Exception { - CustomType1 t = createCustomType1(1); - db.write(t); - - // Entries of a non-copy secondary index hold only the natural key, and next() resolves - // the entity with a live db.get() lookup. - try (KVStoreIterator it = - db.view(CustomType1.class).index("id").closeableIterator()) { - assertTrue(it.hasNext()); - // Delete the entity after hasNext() buffered its index entry: the iterator still - // sees the entry, but the live lookup in next() misses the key. - db.delete(t.getClass(), t.key); - NoSuchElementException e = assertThrows(NoSuchElementException.class, it::next); - assertTrue(e.getMessage().contains(t.key)); - } + public void testDeleteEdgeCases() throws Exception { + // Never-written type: type info is created on the fly, lookup misses, nothing happens. + db.delete(CustomType1.class, "missing"); + assertEquals(0L, db.count(CustomType1.class)); + + // Never-written key of a written type. + db.write(createCustomType1(1)); + db.delete(CustomType1.class, "missing"); + assertEquals(1L, db.count(CustomType1.class)); + + // Mismatched key type: the encoded lookup key misses, nothing is removed. + db.delete(CustomType1.class, 42); + assertEquals(1L, db.count(CustomType1.class)); } @Test 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 9d80c2fabd956..0a382d43b3c6b 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 @@ -96,35 +96,34 @@ public void testObjectWriteReadDelete() throws Exception { } @Test - public void testGetMissingKeyReturnsNull() throws Exception { - // get() returns null instead of throwing for a missing key, so the write path does not - // pay the cost of building an exception when the entry does not exist yet; read() still - // surfaces a missing key as NoSuchElementException. + public void testGetOrNullMissingKey() throws Exception { + // getOrNull() returns null for a missing key so expected misses (e.g. the write path + // looking up an existing entry) skip the cost of building an exception, while get() + // still surfaces a missing key as NoSuchElementException. byte[] missingKey = db.getTypeInfo(CustomType1.class).naturalIndex().start(null, "missing"); - assertNull(db.get(missingKey, CustomType1.class)); + assertNull(db.getOrNull(missingKey, CustomType1.class)); + assertThrows(NoSuchElementException.class, () -> db.get(missingKey, CustomType1.class)); CustomType1 t = createCustomType1(1); db.write(t); byte[] presentKey = db.getTypeInfo(CustomType1.class).naturalIndex().start(null, t.key); - assertEquals(t, db.get(presentKey, CustomType1.class)); + assertEquals(t, db.getOrNull(presentKey, CustomType1.class)); } @Test - public void testNextAfterEntityDelete() throws Exception { - CustomType1 t = createCustomType1(1); - db.write(t); - - // Entries of a non-copy secondary index hold only the natural key, and next() resolves - // the entity with a live db.get() lookup. - try (KVStoreIterator it = - db.view(CustomType1.class).index("id").closeableIterator()) { - assertTrue(it.hasNext()); - // Delete the entity after hasNext() buffered its index entry: the iterator still - // sees the entry, but the live lookup in next() misses the key. - db.delete(t.getClass(), t.key); - NoSuchElementException e = assertThrows(NoSuchElementException.class, it::next); - assertTrue(e.getMessage().contains(t.key)); - } + public void testDeleteEdgeCases() throws Exception { + // Never-written type: type info is created on the fly, lookup misses, nothing happens. + db.delete(CustomType1.class, "missing"); + assertEquals(0L, db.count(CustomType1.class)); + + // Never-written key of a written type. + db.write(createCustomType1(1)); + db.delete(CustomType1.class, "missing"); + assertEquals(1L, db.count(CustomType1.class)); + + // Mismatched key type: the encoded lookup key misses, nothing is removed. + db.delete(CustomType1.class, 42); + assertEquals(1L, db.count(CustomType1.class)); } @Test From 57e3eaca776d8d8559c5528e0bade63482b727b2 Mon Sep 17 00:00:00 2001 From: Xiduo You Date: Thu, 3 Sep 2026 11:58:37 +0800 Subject: [PATCH 4/4] [CORE] Route delete() through getOrNull and fix KVStore.delete javadoc delete() reads the existing entry through getOrNull, getOrNull drops the @VisibleForTesting annotation, and KVStore.delete javadoc states that deleting a missing key is a no-op. The new tests assert through the public API. Assisted-by: Claude Opus 4.8 --- .../java/org/apache/spark/util/kvstore/KVStore.java | 2 +- .../java/org/apache/spark/util/kvstore/LevelDB.java | 6 ++---- .../java/org/apache/spark/util/kvstore/RocksDB.java | 6 ++---- .../org/apache/spark/util/kvstore/LevelDBSuite.java | 11 +++-------- .../org/apache/spark/util/kvstore/RocksDBSuite.java | 11 +++-------- 5 files changed, 11 insertions(+), 25 deletions(-) diff --git a/common/kvstore/src/main/java/org/apache/spark/util/kvstore/KVStore.java b/common/kvstore/src/main/java/org/apache/spark/util/kvstore/KVStore.java index abadbfb4d997b..959e5b1fd1ec6 100644 --- a/common/kvstore/src/main/java/org/apache/spark/util/kvstore/KVStore.java +++ b/common/kvstore/src/main/java/org/apache/spark/util/kvstore/KVStore.java @@ -104,11 +104,11 @@ public interface KVStore extends Closeable { /** * Removes an object and all data related to it, like index entries, from the store. + * Deleting a missing key is a no-op. * * @param type The object's type. * @param naturalKey The object's "natural key", which uniquely identifies it. Null keys * are not allowed. - * @throws java.util.NoSuchElementException If an element with the given key does not exist. */ void delete(Class type, Object naturalKey) throws Exception; diff --git a/common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java b/common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java index daf86837031c5..57d26d09b52ad 100644 --- a/common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java +++ b/common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java @@ -132,7 +132,6 @@ T get(byte[] key, Class klass) throws Exception { * callers where a missing key is expected do not pay the cost of throwing and filling in an * exception stack trace. */ - @VisibleForTesting T getOrNull(byte[] key, Class klass) throws Exception { byte[] data = db().get(key); return data != null ? serializer.deserialize(data, klass) : null; @@ -227,9 +226,8 @@ public void delete(Class type, Object naturalKey) throws Exception { LevelDBTypeInfo ti = getTypeInfo(type); byte[] key = ti.naturalIndex().start(null, naturalKey); synchronized (ti) { - byte[] data = db().get(key); - if (data != null) { - Object existing = serializer.deserialize(data, type); + Object existing = getOrNull(key, type); + if (existing != null) { PrefixCache cache = new PrefixCache(existing); byte[] keyBytes = ti.naturalIndex().toKey(ti.naturalIndex().getValue(existing)); for (LevelDBTypeInfo.Index idx : ti.indices()) { diff --git a/common/kvstore/src/main/java/org/apache/spark/util/kvstore/RocksDB.java b/common/kvstore/src/main/java/org/apache/spark/util/kvstore/RocksDB.java index fadb316bfd8a8..cd4e18a68a2ee 100644 --- a/common/kvstore/src/main/java/org/apache/spark/util/kvstore/RocksDB.java +++ b/common/kvstore/src/main/java/org/apache/spark/util/kvstore/RocksDB.java @@ -165,7 +165,6 @@ T get(byte[] key, Class klass) throws Exception { * callers where a missing key is expected do not pay the cost of throwing and filling in an * exception stack trace. */ - @VisibleForTesting T getOrNull(byte[] key, Class klass) throws Exception { byte[] data = db().get(key); return data != null ? serializer.deserialize(data, klass) : null; @@ -259,9 +258,8 @@ public void delete(Class type, Object naturalKey) throws Exception { RocksDBTypeInfo ti = getTypeInfo(type); byte[] key = ti.naturalIndex().start(null, naturalKey); synchronized (ti) { - byte[] data = db().get(key); - if (data != null) { - Object existing = serializer.deserialize(data, type); + Object existing = getOrNull(key, type); + if (existing != null) { PrefixCache cache = new PrefixCache(existing); byte[] keyBytes = ti.naturalIndex().toKey(ti.naturalIndex().getValue(existing)); for (RocksDBTypeInfo.Index idx : ti.indices()) { 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 057bc7cd20b27..79db74c383993 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 @@ -101,16 +101,15 @@ public void testObjectWriteReadDelete() throws Exception { @Test public void testGetOrNullMissingKey() throws Exception { // getOrNull() returns null for a missing key so expected misses (e.g. the write path - // looking up an existing entry) skip the cost of building an exception, while get() + // looking up an existing entry) skip the cost of building an exception, while read() // still surfaces a missing key as NoSuchElementException. byte[] missingKey = db.getTypeInfo(CustomType1.class).naturalIndex().start(null, "missing"); assertNull(db.getOrNull(missingKey, CustomType1.class)); - assertThrows(NoSuchElementException.class, () -> db.get(missingKey, CustomType1.class)); + assertThrows(NoSuchElementException.class, () -> db.read(CustomType1.class, "missing")); CustomType1 t = createCustomType1(1); db.write(t); - byte[] presentKey = db.getTypeInfo(CustomType1.class).naturalIndex().start(null, t.key); - assertEquals(t, db.getOrNull(presentKey, CustomType1.class)); + assertEquals(t, db.read(CustomType1.class, t.key)); } @Test @@ -123,10 +122,6 @@ public void testDeleteEdgeCases() throws Exception { db.write(createCustomType1(1)); db.delete(CustomType1.class, "missing"); assertEquals(1L, db.count(CustomType1.class)); - - // Mismatched key type: the encoded lookup key misses, nothing is removed. - db.delete(CustomType1.class, 42); - assertEquals(1L, db.count(CustomType1.class)); } @Test 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 0a382d43b3c6b..8de0ea3d8576e 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 @@ -98,16 +98,15 @@ public void testObjectWriteReadDelete() throws Exception { @Test public void testGetOrNullMissingKey() throws Exception { // getOrNull() returns null for a missing key so expected misses (e.g. the write path - // looking up an existing entry) skip the cost of building an exception, while get() + // looking up an existing entry) skip the cost of building an exception, while read() // still surfaces a missing key as NoSuchElementException. byte[] missingKey = db.getTypeInfo(CustomType1.class).naturalIndex().start(null, "missing"); assertNull(db.getOrNull(missingKey, CustomType1.class)); - assertThrows(NoSuchElementException.class, () -> db.get(missingKey, CustomType1.class)); + assertThrows(NoSuchElementException.class, () -> db.read(CustomType1.class, "missing")); CustomType1 t = createCustomType1(1); db.write(t); - byte[] presentKey = db.getTypeInfo(CustomType1.class).naturalIndex().start(null, t.key); - assertEquals(t, db.getOrNull(presentKey, CustomType1.class)); + assertEquals(t, db.read(CustomType1.class, t.key)); } @Test @@ -120,10 +119,6 @@ public void testDeleteEdgeCases() throws Exception { db.write(createCustomType1(1)); db.delete(CustomType1.class, "missing"); assertEquals(1L, db.count(CustomType1.class)); - - // Mismatched key type: the encoded lookup key misses, nothing is removed. - db.delete(CustomType1.class, 42); - assertEquals(1L, db.count(CustomType1.class)); } @Test