LuciferYang commented on code in PR #58466:
URL: https://github.com/apache/spark/pull/58466#discussion_r3914357681
##########
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();
Review Comment:
The `NoSuchElementException` added here drops the key the old exception
carried: `db.get` used to throw with `new String(key, UTF_8)` as the message,
and `read()` keeps exactly that (LevelDB.java:146), so the same case is handled
two different ways in one PR. When an entity is deleted between `hasNext()` and
`next()`, the exception no longer says which key went missing, which is a real
loss when debugging store-level inconsistencies.
Please use `throw new NoSuchElementException(new String(key, UTF_8));` in
both iterators; each file also needs `import static
java.nio.charset.StandardCharsets.UTF_8;`.
##########
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();
Review Comment:
Same as my comment on LevelDBIterator.java — please carry the key in the
message here too.
##########
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 {
Review Comment:
The new null check in both iterators is not exercised by any test: `read()`
is covered by existing assertions and `get()`'s null contract by the new test,
but this branch is a blind spot. You can hit it deterministically: call
`hasNext()` on a non-copy secondary index, delete the entity via
`store.delete()`, then assert `next()` throws `NoSuchElementException`.
A test next to `testGetMissingKeyReturnsNull` would do; not a merge blocker.
--
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]
