HDDS-16463. Sync Recon derived DB before cursor advance - #11262
Open
HesandaLiyanage wants to merge 10 commits into
Open
HesandaLiyanage wants to merge 10 commits into
HesandaLiyanage wants to merge 10 commits into
Conversation
During delta sync, ReconTaskControllerImpl processes tasks that write derived data to RocksDB (with sync=false by default) and then commits the task cursor to Derby (which is fsync-durable). A power loss in this window can lose the un-synced RocksDB WAL tail while Derby records completion at sequence N. On startup, reconciliation sees equal sequence numbers and skips reprocessing, causing permanent loss of the update. This patch introduces a write-time durability barrier by syncing the Recon DB WAL (DBStore.flushLog(true)) before advancing the task status cursor in Derby. If the sync fails, the cursor remains unadvanced and the task is marked for retry/reprocessing.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The retry list has a concurrency race, and tests do not verify sync ordering or failure handling.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a RocksDB WAL sync barrier so Recon derived data is durable before task cursors advance.
Changes:
- Syncs
recon.dbWAL before recording successful task completion. - Retries tasks when WAL syncing fails.
- Adds durability-related unit coverage.
File summaries
| File | Description |
|---|---|
ReconTaskControllerImpl.java |
Implements WAL synchronization and failure handling. |
TestReconTaskControllerImpl.java |
Adds sync-barrier test coverage. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Coalesce the RocksDB WAL flushLog(true) durability barrier to execute once per batch after all tasks complete their execution, instead of syncing per successful task. This avoids multiple redundant fsync calls and thread contention on the same database while preserving the guarantee that derived data is durable before task cursors are committed to Derby.
Ensure that failed task tracking during parallel execution is thread-safe. Collect task execution failures in a thread-safe synchronized list during asynchronous execution, sequentially transferring them to failedTasks after futures complete. Additionally, initialize failedTasks and retryFailedTasks using synchronizedList in processOMUpdateBatch to prevent race conditions on concurrent updates.
Add testDerivedDbSyncFailureLeavesCursorUnadvancedAndRetries to verify the durability barrier failure behavior when DBStore.flushLog(true) throws RocksDatabaseException. Asserts that the task cursor remains unadvanced, the task status is marked as failed (-1), and the task is retried through the batch retry path.
… commit Use Mockito doAnswer on reconDbStore.flushLog(true) to verify that at the exact moment the WAL sync is invoked, the task cursors in Derby have not yet advanced to the batch sequence number. Asserts causal ordering of the durability barrier rather than just eventual invocation.
chihsuan
reviewed
Sep 19, 2026
chihsuan
left a comment
chihsuan
left a comment
Contributor
There was a problem hiding this comment.
Thanks for working on this! @HesandaLiyanage 👍 I left three inline notes. Also, could we trim the rationale to one place (the syncReconDbLog javadoc)? It appears three times now, and the test javadoc is the only one in that file.
- Signal task reinitialization on sync failure instead of retrying process() to avoid double counting - Record delta processing failure in taskMetrics on sync failure - Return false when dbStore is null in syncReconDbLog() - Trim rationale comments to canonical syncReconDbLog() javadoc - Update unit tests to verify reinitialization signaling without process() retry and null dbStore handling
Author
|
Hi @chihsuan , thank you so much for the review! I have addressed all the feedback in the latest commit:
Please let me know if there's anything else! |
chihsuan
reviewed
Sep 20, 2026
chihsuan
left a comment
chihsuan
left a comment
Contributor
There was a problem hiding this comment.
Thanks @HesandaLiyanage! Left two small nits inline, but overall looks good.
chihsuan
approved these changes
Sep 20, 2026
Both failedTasks and retryFailedTasks are only accessed on the calling thread after joining parallel task execution, so synchronization is unnecessary.
Fold the separate success and failure loops into a single loop keyed on the syncReconDbLog result, reducing duplicated iteration and updater boilerplate.
Author
|
@chihsuan Fixed the nit comments |
Author
|
@ivandika3 @chihsuan ping |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
During Recon delta sync,
ReconTaskControllerImpl.processTasks()executes derived tasks (e.g.ContainerKeyMapperTask,NSSummaryTask), which write derived data to Recon's local RocksDB (recon.db). By default, RocksDB writes are not synced (sync = false) and remain in the OS page cache. Immediately following task success,recordRunCompletion()commits the task cursor (lastUpdatedSeqNumberandlastTaskRunStatus = 0) to Derby, which issues anfsync-durable commit.If a host power loss occurs in that window, the un-synced RocksDB WAL tail at sequence$N$ is lost while the durable Derby cursor survives at sequence $N$ . On restart, $N$ , it skips reprocessing. The next delta sync starts from sequence $N$ , permanently dropping the applied update from Recon's derived tables.
OzoneManagerServiceProviderImpl.start()compares the task cursor with the delta cursor; because both are at sequenceThis pull request introduces a write-time durability barrier:
syncReconDbLog()inReconTaskControllerImplto flush and sync the derived RocksDB WAL viaDBStore.flushLog(true)before advancing and committing task status cursors.fsynccalls and thread contention on the database across parallel tasks.failedTasks, and ensuresprocessOMUpdateBatchuses synchronized collections.syncReconDbLog()fails (e.g.,RocksDatabaseException), leaves the cursors unadvanced, markslastTaskRunStatusas-1, and registers the tasks infailedTasksfor immediate retry/reprocessing.TestReconTaskControllerImpl:testDerivedDbSyncedBeforeCursorAdvanceOnSuccess: asserts thatflushLog(true)is called strictly before Derby cursors advance to sequence 100L, coalescing multiple task writes into a single sync per batch.testDerivedDbSyncFailureLeavesCursorUnadvancedAndRetries: asserts that ifflushLog(true)throwsRocksDatabaseException, cursors remain unadvanced at 0L, run status is marked -1, and the batch retry path is exercised.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16463
How was this patch tested?
mvn -pl :ozone-recon -am test -Dtest=TestReconTaskControllerImpl -DskipShade -DskipRecon -DskipDocs