[fix](editlog) Fix BDBEnvironment.removeDatabase() wrong list removal index#62064
[fix](editlog) Fix BDBEnvironment.removeDatabase() wrong list removal index#62064dataroaring wants to merge 1 commit intoapache:masterfrom
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
There was a problem hiding this comment.
Pull request overview
Fixes BDBEnvironment.removeDatabase() so it removes the correct database handle from openedDatabases even if getDatabaseName() throws during iteration.
Changes:
- Replaced manual index tracking + post-loop
openedDatabases.remove(index)withIterator.remove()during iteration. - Simplified removal flow by eliminating the
targetDbName/index bookkeeping.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| String targetDbName = null; | ||
| int index = 0; | ||
| for (Database db : openedDatabases) { | ||
| for (java.util.Iterator<Database> iter = openedDatabases.iterator(); iter.hasNext();) { |
There was a problem hiding this comment.
Using a fully-qualified type (java.util.Iterator) inside method bodies hurts readability and is inconsistent with typical Java style. Prefer importing java.util.Iterator (or using an existing import) and writing for (Iterator<Database> iter = ...) instead.
There was a problem hiding this comment.
Done. Added import and changed to Iterator.
|
/review |
…t removal removeDatabase() uses a manual index counter to track position in openedDatabases, then calls openedDatabases.remove(index). But the index variable is only incremented when the database name does NOT match. When the match is found and the loop breaks, index correctly points to the matched element. However, this pattern is fragile and error-prone compared to using iterator.remove(). More critically, if getDatabaseName() throws (e.g. DatabasePreemptedException for a preempted database), the index counter gets out of sync with the actual position, causing removal of the wrong database handle from the list. Fix: replace the manual index tracking with iterator.remove(), which is both correct and concise.
dataroaring
force-pushed
the
fix/bdbenv-remove-index
branch
from
138ab56 to
ba6c240
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Found 1 issue that should be addressed before this change lands.
Critical checkpoint conclusions:
- Goal of the task: Partially met. The patch removes the fragile manual index bookkeeping, but it does not handle the documented
DatabasePreemptedException/stale-handle path that motivated the bug report, soremoveDatabase()can still fail before removing the target DB. No test proves this path. - Change size/focus: Yes. The patch is small and focused.
- Concurrency:
openedDatabasesis still protected by the existing write lock, and this patch does not introduce a new lock-order issue. - Lifecycle/static init: No special lifecycle or static initialization changes.
- Configuration: No config changes.
- Compatibility: No FE/BE protocol or storage compatibility impact.
- Parallel paths: Not fully handled.
openDatabase()already treats preempted databases as expected stale state, butremoveDatabase()still does not mirror that behavior. - Special conditions: The special preempted-database condition is documented in the same class, but this method still lacks the required exception handling.
- Test coverage: Insufficient for the changed bug path. Existing tests cover normal removal and removing twice, not a preempted/stale handle in
openedDatabases. - Observability: Existing logging is adequate for this path.
- Transaction/persistence: The change touches journal database cleanup; if
removeDatabase()aborts early, old journal DBs can remain undeleted. - Data write/modification correctness: Cleanup correctness is still at risk because a stale handle can stop removal of the intended DB.
- FE/BE variable passing: Not applicable.
- Performance: No material performance concern in this patch.
- Other issues: None beyond the inline comment.
Overall opinion: needs follow-up before merge because the reported stale-handle scenario is still not fully handled.
| for (Database db : openedDatabases) { | ||
| for (java.util.Iterator<Database> iter = openedDatabases.iterator(); iter.hasNext();) { | ||
| Database db = iter.next(); | ||
| String name = db.getDatabaseName(); |
There was a problem hiding this comment.
iter.remove() fixes the index bookkeeping bug, but this line can still throw on a stale/preempted handle and abort the whole method. The same class already documents this exact DatabasePreemptedException case in openDatabase().
Concrete failure path: if openedDatabases contains a handle that was preempted by a replicated remove, db.getDatabaseName() throws here, removeDatabase() exits before calling replicatedEnvironment.removeDatabase(null, dbName), and the old journal DB is not removed.
Please mirror the stale-handle cleanup pattern from openDatabase() here (catch the exception, remove the stale entry, and continue scanning), and add a unit test that covers a preempted database handle in openedDatabases.
Summary
removeDatabase()uses a manualindexcounter during iteration, then callsopenedDatabases.remove(index)after the loopgetDatabaseName()throws (e.g.DatabasePreemptedExceptionfor a preempted database), the index counter gets out of sync with actual position, causing removal of the wrong database handleiterator.remove(), which is both correct and conciseTest plan
openedDatabases🤖 Generated with Claude Code