dongjoon-hyun commented on PR #58466:
URL: https://github.com/apache/spark/pull/58466#issuecomment-5510519855
Thank you for the PR. I went through the diff and the surrounding code paths
in `common/kvstore` and the History Server callers. I didn't find any
correctness issue: every removed `try/catch` is re-established by a null check,
and `read()` / iterator `next()` still throw the same `NoSuchElementException`
with the same message as before. A few non-blocking comments below.
**Design / simplification**
1. **Consider keeping the throwing `get()` and adding a null-returning
sibling instead.** Only three callers ever swallowed the
`NoSuchElementException` (constructor, `getMetadata`, `updateBatch`), and
`updateBatch` is the hot path. If we keep `get()` as-is and add e.g.
`getOrNull(byte[], Class)` for those three, `read()`, `RocksDBIterator` and
`LevelDBIterator` need no change at all, and the missing-key exception is built
in one place per store instead of four (`RocksDB.read`, `LevelDB.read`,
`RocksDBIterator.next`, `LevelDBIterator.next`). It also drops the two new
`UTF_8` static imports from the iterators.
2. **`delete()` still has a dead `catch (NoSuchElementException nse) { //
Ignore. }`** (`RocksDB.java:271`, `LevelDB.java:239`). Nothing in that try body
can throw it (it uses the raw `db().get(key)` with a null guard). It was
already dead before this PR, but since this PR removes every other NSEE catch
in these files and documents that `get()` returns null, it would be nice to
drop it here too.
3. **`testNextAfterEntityDelete` hardens "`hasNext()` is true, then `next()`
throws `NoSuchElementException`" as the contract.** That is the pre-existing
behavior, so no objection to the test itself, just noting that NSEE from
`Iterator.next()` conventionally means exhaustion, and in the live-UI RocksDB
store case a concurrent delete by `ElementTrackingStore` would surface to the
REST layer as a 404. Skipping dangling index entries in `loadNext()` (or using
a distinct exception type) could be a follow-up.
**Efficiency (pre-existing, on the same write path; fine as follow-ups)**
4. `updateBatch` computes the natural key three times per write:
`naturalIndex.entityKey(null, value)`, then
`naturalIndex.toKey(naturalIndex.getValue(value))`, then again inside
`Index.addOrRemove` for the natural index. Computing `naturalKey` first and
building the lookup key with `ti.buildKey(false, naturalIndex.keyPrefix(null),
naturalKey)` (the same construction `RocksDBIterator.next` uses) would remove
two reflective accessor calls and two encodings per write.
5. `updateBatch` deserializes the existing entity even for
natural-index-only types (`AppSummary`, `PoolData`, `SparkPlanGraphWrapper`,
...), where `existing` is only used as a presence flag. A raw `db().get` null
check would suffice there.
6. In `next()`, `ti.naturalIndex().keyPrefix(null)` allocates a fresh prefix
on every element although it is constant for the iterator's lifetime; the
constructor already caches `indexKeyPrefix` the same way.
7. Outside this PR: `FsHistoryProvider` still uses `listing.read(...)` +
`catch NoSuchElementException` as the "not tracked yet" check for every newly
discovered log (`checkForLogs`, `checkAndCleanLog`, `cleanDriverLogs`,
`addListing`). It is orders of magnitude smaller than the `updateBatch` cost
fixed here, but a null-returning read exposed through `KVStore` would remove
it. Probably a separate JIRA.
**PR description**
8. The template asks for `Generated-by:` followed by the tool name **and its
version**; the body currently says only `Claude Code`, while the commit trailer
records `Assisted-by: Claude Opus 4.8`. Also, the "How was this patch tested?"
section still lists only `testGetMissingKeyReturnsNull` and does not mention
`testNextAfterEntityDelete` or the iterator message change from the second
commit.
<sub>Reviewed with the help of Claude Fable 5.1.</sub>
--
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]
