RexXiong commented on PR #3695:
URL: https://github.com/apache/celeborn/pull/3695#issuecomment-4552673069
Thanks for the work on this PR — the overall design (ManagedRocksDB
lifecycle wrapper, generation + ReadWriteLock for concurrent recovery dedup,
stale iterator detection) is solid. A couple of observations:
**1. `tryRecoverDBInstance`: failed reopen leaves DB in an infinite recovery
loop**
If `reopenRocksDB` throws, `db` still references the already-closed
`ManagedRocksDB`, and the generation has already been incremented:
```java
try {
if (db != null) {
db.close(); // old DB closed
}
} catch (Exception e) { ... }
dbGeneration.incrementAndGet(); // generation bumped
try {
db = RocksDBProvider.reopenRocksDB(dbFile, conf); // if this fails...
} catch (IOException e) {
logger.error("Safe reopen failed ...", e);
// db still points to the CLOSED ManagedRocksDB
// generation already incremented → dedup won't block next attempt
}
```
Every subsequent operation will: use the closed DB → throw → trigger a new
recovery (passes the dedup check since generation advanced) → close the
already-closed DB → attempt reopen → fail again. Each operation pays the cost
of a write-lock acquisition + a full reopen attempt, and it never converges.
Suggestion: mark the DB as terminally failed after reopen failure so that
`checkState()` fast-fails with a clear message (e.g., "DB recovery failed,
manual intervention required") instead of looping:
```java
} catch (IOException e) {
logger.error("Safe reopen failed for RocksDB at {}.", dbFile, e);
db = null;
closed = true; // or a dedicated recoveryFailed flag
}
```
**2. Minor: `testNewIteratorReturnsUsableIterator` assertion is fragile**
The test puts 2 entries ("a", "b") but asserts `seen >= 3`, relying on the
version entry written internally by `initRockDB`. If the version storage
mechanism changes, this test will break. Consider asserting `seen >= 2` or
using an exact count.
*Reviewed with Claude Code*
--
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]
