You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "HesandaLiyanage (via GitHub)" <gi...@apache.org> on 2026/09/18 19:29:58 UTC
[PR] HDDS-16463. Sync Recon derived DB before cursor advance [ozone]
HesandaLiyanage opened a new pull request, #11262:
URL: https://github.com/apache/ozone/pull/11262
## 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 (`lastUpdatedSeqNumber` and `lastTaskRunStatus = 0`) to Derby, which issues an `fsync`-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, `OzoneManagerServiceProviderImpl.start()` compares the task cursor with the delta cursor; because both are at sequence $N$, it skips reprocessing. The next delta sync starts from sequence $N$, permanently dropping the applied update from Recon's derived tables.
This pull request introduces a write-time durability barrier:
1. Adds `syncReconDbLog()` in `ReconTaskControllerImpl` to sync the derived RocksDB WAL via `DBStore.flushLog(true)` before advancing and committing the task status cursor.
2. If `syncReconDbLog()` fails (e.g., `RocksDatabaseException`), leaves the cursor unadvanced, marks `lastTaskRunStatus` as `-1`, and registers the task in `failedTasks` for retry/reprocessing.
3. Adds unit test `testDerivedDbSyncedBeforeCursorAdvanceOnSuccess` to `TestReconTaskControllerImpl` asserting that `flushLog(true)` is called before the cursor advances and `flushLog(false)` is never called.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16463
## How was this patch tested?
1. Unit tests:
```bash
mvn -pl :ozone-recon test -Dtest=TestReconTaskControllerImpl -DskipShade -DskipRecon -DskipDocs
```
All 22 tests passed (0 failures, 0 errors).
2. Checkstyle:
```bash
./hadoop-ozone/dev-support/checks/checkstyle.sh
```
0 violations across all 58 modules.
---
Generated-by: Antigravity (Gemini 3.8 Flash)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16463. Sync Recon derived DB before cursor advance [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11262:
URL: https://github.com/apache/ozone/pull/11262#issuecomment-6329392574
Thank you for your contribution. This PR is being closed due to inactivity. Please contact a maintainer if you would like to reopen it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16463. Sync Recon derived DB before cursor advance [ozone]
Posted by "HesandaLiyanage (via GitHub)" <gi...@apache.org>.
HesandaLiyanage commented on PR #11262:
URL: https://github.com/apache/ozone/pull/11262#issuecomment-5739918013
Hi @chihsuan , thank you so much for the review!
I have addressed all the feedback in the latest commit:
- Signaled task reinitialization (`tasksFailed`) directly on sync failure instead of retrying `process()`, preventing double-counting in incremental counting tasks.
- Recorded failures in `taskMetrics` when sync fails.
- Handled `dbStore == null` by returning `false` so cursors never advance without durability guarantees.
- Trimmed the rationale comments down to the canonical `syncReconDbLog()` javadoc.
- Updated the unit tests to verify that `process()` is not retried on sync failure, and added a test for the null `dbStore` path.
Please let me know if there's anything else!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16463. Sync Recon derived DB before cursor advance [ozone]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #11262:
URL: https://github.com/apache/ozone/pull/11262#discussion_r4050087562
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ReconTaskControllerImpl.java:
##########
@@ -473,8 +475,23 @@ private void processTasks(
// Track task delta processing success
taskMetrics.incrTaskDeltaProcessingSuccess(taskName);
- taskStatusUpdater.setLastTaskRunStatus(0);
- taskStatusUpdater.setLastUpdatedSeqNumber(events.getLastSequenceNumber());
+ // Make the derived-table RocksDB writes for this batch durable before the task-status
+ // cursor is committed to Derby. The cursor row is fsync-durable while the RocksDB writes
+ // are not synced by default, so without this barrier a power loss can leave the durable
+ // cursor ahead of the (lost) derived data. Startup reconciliation then sees the derived
+ // and delta cursors at the same sequence number and never reprocesses, permanently
+ // dropping the applied update. If the sync fails, leave the cursor unadvanced so the
+ // batch is reprocessed instead of recording an un-durable success.
+ if (syncReconDbLog()) {
+ taskStatusUpdater.setLastTaskRunStatus(0);
+ taskStatusUpdater.setLastUpdatedSeqNumber(events.getLastSequenceNumber());
+ } else {
+ failedTasks.add(new ReconOmTask.TaskResult.Builder()
+ .setTaskName(taskName)
+ .setSubTaskSeekPositions(result.getSubTaskSeekPositions())
+ .build());
Review Comment:
`failedTasks` is an `ArrayList`, but these completion callbacks run concurrently on `executorService`. If multiple tasks hit this new sync-failure branch, concurrent `add` calls can lose entries, so some failed tasks may not be retried. Make the collection thread-safe (and apply the same fix to the existing add at line 469).
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ReconTaskControllerImpl.java:
##########
@@ -503,6 +520,27 @@ private void processTasks(
LOG.error("Some tasks were cancelled with exception", ce);
}
}
+
+ /**
+ * Flushes and syncs the Recon derived-data RocksDB write-ahead log to stable storage so the
+ * derived writes for the processed batch are durable before the task-status cursor advances.
+ * Returns {@code false} if the sync fails, in which case the caller must not advance the cursor,
+ * so that the cursor is never persisted ahead of the derived data.
+ */
+ private boolean syncReconDbLog() {
+ DBStore dbStore = reconDBProvider.getDbStore();
+ if (dbStore == null) {
+ return true;
+ }
+ try {
+ dbStore.flushLog(true);
+ return true;
+ } catch (RocksDatabaseException e) {
+ LOG.error("Failed to sync Recon DB WAL before advancing task status cursor; "
+ + "leaving cursor unadvanced so the batch is reprocessed.", e);
+ return false;
Review Comment:
The failure behavior introduced here is not covered: no test makes `flushLog(true)` throw and verifies that the task cursor stays at its previous sequence, status becomes `-1`, and the retry path is exercised. This is the critical failure mode of the durability barrier and should be protected by a unit test.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ReconTaskControllerImpl.java:
##########
@@ -473,8 +475,23 @@ private void processTasks(
// Track task delta processing success
taskMetrics.incrTaskDeltaProcessingSuccess(taskName);
- taskStatusUpdater.setLastTaskRunStatus(0);
- taskStatusUpdater.setLastUpdatedSeqNumber(events.getLastSequenceNumber());
+ // Make the derived-table RocksDB writes for this batch durable before the task-status
+ // cursor is committed to Derby. The cursor row is fsync-durable while the RocksDB writes
+ // are not synced by default, so without this barrier a power loss can leave the durable
+ // cursor ahead of the (lost) derived data. Startup reconciliation then sees the derived
+ // and delta cursors at the same sequence number and never reprocesses, permanently
+ // dropping the applied update. If the sync fails, leave the cursor unadvanced so the
+ // batch is reprocessed instead of recording an un-durable success.
+ if (syncReconDbLog()) {
+ taskStatusUpdater.setLastTaskRunStatus(0);
+ taskStatusUpdater.setLastUpdatedSeqNumber(events.getLastSequenceNumber());
Review Comment:
This callback runs once per successful task, so a batch with the six registered Recon tasks can issue multiple `flushLog(true)` calls, each potentially forcing an fsync on the same WAL. That adds avoidable durable-sync latency and blocks task executor threads; coalesce the barrier after all task writes complete, then commit the successful task cursors, or otherwise share one sync per batch.
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/tasks/TestReconTaskControllerImpl.java:
##########
@@ -932,6 +935,46 @@ public void testCreateOMCheckpointThrowsWhenCheckpointNull() throws Exception {
assertThrows(IOException.class, () -> controller.createOMCheckpoint(omMetadataManager));
}
+ /**
+ * A successful delta batch must sync the derived-data RocksDB WAL to stable storage before the
+ * fsync-durable task-status cursor is advanced. Without that barrier a power loss can lose the
+ * un-synced derived write while the durable cursor survives, leaving the cursor ahead of the
+ * data; startup reconciliation then sees equal cursors and never reprocesses, permanently
+ * dropping the applied update.
+ */
+ @Test
+ public void testDerivedDbSyncedBeforeCursorAdvanceOnSuccess() throws Exception {
+ ReconOmTask reconOmTaskMock = getMockTask("SyncBarrierTask");
+ when(reconOmTaskMock.process(any(OMUpdateEventBatch.class), anyMap()))
+ .thenReturn(new ReconOmTask.TaskResult.Builder()
+ .setTaskName("SyncBarrierTask").setTaskSuccess(true).build());
+ reconTaskController.registerTask(reconOmTaskMock);
+
+ OMUpdateEventBatch batch = mock(OMUpdateEventBatch.class);
+ when(batch.getLastSequenceNumber()).thenReturn(100L);
+ when(batch.isEmpty()).thenReturn(false);
+ when(batch.getEvents()).thenReturn(new ArrayList<>());
+ when(batch.getEventType()).thenReturn(ReconEvent.EventType.OM_UPDATE_BATCH);
+ when(batch.getEventCount()).thenReturn(1);
+
+ reconTaskController.consumeOMEvents(batch, mock(OMMetadataManager.class));
+
+ GenericTestUtils.waitFor(() -> {
+ try {
+ ReconTaskStatus status = reconTaskStatusDao.findById("SyncBarrierTask");
+ return status != null && status.getLastTaskRunStatus() == 0
+ && status.getLastUpdatedSeqNumber() == 100L;
+ } catch (Exception e) {
+ return false;
+ }
+ }, 100, 5000);
+
+ // The derived-data RocksDB WAL must be synced (sync == true) before the durable cursor advances.
+ verify(reconDbStore, times(1)).flushLog(true);
+ // A non-syncing flush would leave the write in page cache and reintroduce the durability gap.
+ verify(reconDbStore, never()).flushLog(false);
Review Comment:
The new test only verifies that `flushLog(true)` was eventually called after observing the committed status. A regression that calls `recordRunCompletion()` first and then flushes could still pass if the flush wins this race, so the required ordering is not actually asserted. Use an ordered verification or record the flush and cursor-update events explicitly.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16463. Sync Recon derived DB before cursor advance [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #11262: HDDS-16463. Sync Recon derived DB before cursor advance
URL: https://github.com/apache/ozone/pull/11262
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16463. Sync Recon derived DB before cursor advance [ozone]
Posted by "HesandaLiyanage (via GitHub)" <gi...@apache.org>.
HesandaLiyanage commented on PR #11262:
URL: https://github.com/apache/ozone/pull/11262#issuecomment-5748463686
@chihsuan Fixed the nit comments
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16463. Sync Recon derived DB before cursor advance [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11262:
URL: https://github.com/apache/ozone/pull/11262#issuecomment-6181248928
This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16463. Sync Recon derived DB before cursor advance [ozone]
Posted by "devmadhuu (via GitHub) via issues" <is...@ozone.apache.org>.
devmadhuu commented on PR #11262:
URL: https://github.com/apache/ozone/pull/11262#issuecomment-6686970336
@ArafatKhan2198 this is also important PR to be handled, but review comments are not handled yet. Do you have bandwidth to take care.
cc: @jojochuang @smengcl @umamaheswararao @rakeshadr
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16463. Sync Recon derived DB before cursor advance [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #11262:
URL: https://github.com/apache/ozone/pull/11262#discussion_r4085548000
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ReconTaskControllerImpl.java:
##########
@@ -502,6 +502,61 @@ private void processTasks(
} catch (CancellationException ce) {
LOG.error("Some tasks were cancelled with exception", ce);
}
+
+ failedTasks.addAll(executionFailedTasks);
+
+ if (!successfulTasks.isEmpty()) {
+ // Sync derived-data RocksDB WAL before committing task-status cursors to avoid durability gaps.
+ boolean synced = syncReconDbLog();
Review Comment:
One failed fsync sets tasksFailed, which stops event buffering (`consumeOMEvents`) and queues `TASK_FAILURES` `reinit` — a full staged reprocess of all six tasks with a complete OM DB scan. That's a little too harsh response to a transient I/O hiccup, and the flush isn't retried first. Would a bounded retry on `flushLog` before escalating be worth it?
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ReconTaskControllerImpl.java:
##########
@@ -502,6 +502,61 @@ private void processTasks(
} catch (CancellationException ce) {
LOG.error("Some tasks were cancelled with exception", ce);
}
+
+ failedTasks.addAll(executionFailedTasks);
+
+ if (!successfulTasks.isEmpty()) {
+ // Sync derived-data RocksDB WAL before committing task-status cursors to avoid durability gaps.
+ boolean synced = syncReconDbLog();
+ for (ReconOmTask.TaskResult result : successfulTasks) {
+ String taskName = result.getTaskName();
+ ReconTaskStatusUpdater taskStatusUpdater =
+ taskStatusUpdaterManager.getTaskStatusUpdater(taskName);
+
+ if (synced) {
+ taskMetrics.incrTaskDeltaProcessingSuccess(taskName);
+ taskStatusUpdater.setLastTaskRunStatus(0);
+ taskStatusUpdater.setLastUpdatedSeqNumber(events.getLastSequenceNumber());
+ } else {
+ taskMetrics.incrTaskDeltaProcessingFailures(taskName);
+ taskStatusUpdater.setLastTaskRunStatus(-1);
+ }
+ taskStatusUpdater.recordRunCompletion();
+ }
+
+ if (!synced) {
+ // Signal task reinitialization directly instead of retrying process(),
+ // because task writes were already applied and retrying would re-apply non-idempotent events.
+ tasksFailed.compareAndSet(false, true);
+ }
+ }
+ }
+
+ /**
+ * Flushes and syncs the Recon derived-data RocksDB write-ahead log to stable storage so the
+ * derived writes for the processed batch are durable before the task-status cursor advances.
+ *
+ * The cursor rows committed to Derby are fsync-durable while RocksDB writes are not synced by default.
+ * Without this barrier, a power loss can leave the durable cursors ahead of the (lost) derived data.
+ * On restart, reconciliation sees matching sequence numbers and never reprocesses the events,
+ * permanently dropping the applied updates.
+ *
+ * @return {@code true} if sync succeeded; {@code false} if dbStore is null or sync failed,
+ * in which case callers must not advance cursors and must signal reinitialization.
+ */
+ private boolean syncReconDbLog() {
+ DBStore dbStore = reconDBProvider.getDbStore();
+ if (dbStore == null) {
+ LOG.error("Recon DB store is null; cannot sync WAL before advancing task status cursor.");
+ return false;
+ }
+ try {
+ dbStore.flushLog(true);
Review Comment:
`flushLog(true) `has a side effect on failure. It delegates to `RDBStore.flushLog (RDBStore.java)`, which calls `db.flushWal(sync) `on the underlying `RocksDatabase`. That method calls `closeOnError(e)` before rethrowing `(RocksDatabase.java)`, and `shouldClose` returns true for Corruption and IOError. So on those codes recon.db's RocksDB handle is closed by the time we catch the exception.
The reinit we signal does recover — it reprocesses into a staged DB and replaceStagedDb reopens the live store But during that, Recon's read endpoints will fail with RocksDatabaseException("Rocks Database is closed") and nothing logs why. Could the catch block log that the store may now be closed? It would save real debugging time.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16463. Sync Recon derived DB before cursor advance [ozone]
Posted by "chihsuan (via GitHub)" <gi...@apache.org>.
chihsuan commented on code in PR #11262:
URL: https://github.com/apache/ozone/pull/11262#discussion_r4056047408
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ReconTaskControllerImpl.java:
##########
@@ -554,7 +614,7 @@ private void processOMUpdateBatch(OMUpdateEventBatch events) {
}
Collection<NamedCallableTask<ReconOmTask.TaskResult>> tasks = new ArrayList<>();
- List<ReconOmTask.TaskResult> failedTasks = new ArrayList<>();
+ List<ReconOmTask.TaskResult> failedTasks = Collections.synchronizedList(new ArrayList<>());
Review Comment:
nit: Could these two go back to plain `ArrayList`? I noticed `failedTasks` and `retryFailedTasks` are now only touched on the calling thread after the join. The synchronized wrapper may suggest a concurrent writer that no longer exists.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ReconTaskControllerImpl.java:
##########
@@ -502,6 +502,66 @@ private void processTasks(
} catch (CancellationException ce) {
LOG.error("Some tasks were cancelled with exception", ce);
}
+
+ failedTasks.addAll(executionFailedTasks);
+
+ if (!successfulTasks.isEmpty()) {
+ // Sync derived-data RocksDB WAL before committing task-status cursors to avoid durability gaps.
+ if (syncReconDbLog()) {
+ for (ReconOmTask.TaskResult result : successfulTasks) {
+ String taskName = result.getTaskName();
+ // Track task delta processing success
+ taskMetrics.incrTaskDeltaProcessingSuccess(taskName);
+
+ ReconTaskStatusUpdater taskStatusUpdater =
+ taskStatusUpdaterManager.getTaskStatusUpdater(taskName);
+ taskStatusUpdater.setLastTaskRunStatus(0);
+ taskStatusUpdater.setLastUpdatedSeqNumber(events.getLastSequenceNumber());
+ taskStatusUpdater.recordRunCompletion();
+ }
+ } else {
+ for (ReconOmTask.TaskResult result : successfulTasks) {
+ String taskName = result.getTaskName();
+ // Track task delta processing failure
+ taskMetrics.incrTaskDeltaProcessingFailures(taskName);
+
+ ReconTaskStatusUpdater taskStatusUpdater =
+ taskStatusUpdaterManager.getTaskStatusUpdater(taskName);
+ taskStatusUpdater.setLastTaskRunStatus(-1);
+ taskStatusUpdater.recordRunCompletion();
+ }
+ // Signal task reinitialization directly instead of retrying process(),
+ // because task writes were already applied and retrying would re-apply non-idempotent events.
+ tasksFailed.compareAndSet(false, true);
Review Comment:
nit: Would it be worth folding these two loops into one? They only differ in the status code and the metric bumped, so one loop keyed on the sync result might read easier. I don't feel strongly either way.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16463. Sync Recon derived DB before cursor advance [ozone]
Posted by "chihsuan (via GitHub)" <gi...@apache.org>.
chihsuan commented on code in PR #11262:
URL: https://github.com/apache/ozone/pull/11262#discussion_r4052391717
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ReconTaskControllerImpl.java:
##########
@@ -502,6 +502,65 @@ private void processTasks(
} catch (CancellationException ce) {
LOG.error("Some tasks were cancelled with exception", ce);
}
+
+ failedTasks.addAll(executionFailedTasks);
+
+ if (!successfulTasks.isEmpty()) {
+ // Make the derived-table RocksDB writes for this batch durable before the task-status
+ // cursors are committed to Derby. The cursor rows are fsync-durable while the RocksDB writes
+ // are not synced by default, so without this barrier a power loss can leave the durable
+ // cursors ahead of the (lost) derived data. Startup reconciliation then sees the derived
+ // and delta cursors at the same sequence number and never reprocesses, permanently
+ // dropping the applied update. Coalescing the barrier to once per batch syncs all successful
+ // task writes together before any cursor advances. If the sync fails, leave the cursors
+ // unadvanced so the batch is reprocessed instead of recording an un-durable success.
+ if (syncReconDbLog()) {
+ for (ReconOmTask.TaskResult result : successfulTasks) {
+ String taskName = result.getTaskName();
+ // Track task delta processing success
+ taskMetrics.incrTaskDeltaProcessingSuccess(taskName);
+
+ ReconTaskStatusUpdater taskStatusUpdater =
+ taskStatusUpdaterManager.getTaskStatusUpdater(taskName);
+ taskStatusUpdater.setLastTaskRunStatus(0);
+ taskStatusUpdater.setLastUpdatedSeqNumber(events.getLastSequenceNumber());
+ taskStatusUpdater.recordRunCompletion();
+ }
+ } else {
+ for (ReconOmTask.TaskResult result : successfulTasks) {
+ String taskName = result.getTaskName();
+ failedTasks.add(new ReconOmTask.TaskResult.Builder()
Review Comment:
Should we signal reinit here instead of re-running process()? I noticed the task already committed this batch, so the retry applies the same events twice. With a transient sync failure, a counting task ends up double counted and the cursor still advances.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ReconTaskControllerImpl.java:
##########
@@ -502,6 +502,65 @@ private void processTasks(
} catch (CancellationException ce) {
LOG.error("Some tasks were cancelled with exception", ce);
}
+
+ failedTasks.addAll(executionFailedTasks);
+
+ if (!successfulTasks.isEmpty()) {
+ // Make the derived-table RocksDB writes for this batch durable before the task-status
+ // cursors are committed to Derby. The cursor rows are fsync-durable while the RocksDB writes
+ // are not synced by default, so without this barrier a power loss can leave the durable
+ // cursors ahead of the (lost) derived data. Startup reconciliation then sees the derived
+ // and delta cursors at the same sequence number and never reprocesses, permanently
+ // dropping the applied update. Coalescing the barrier to once per batch syncs all successful
+ // task writes together before any cursor advances. If the sync fails, leave the cursors
+ // unadvanced so the batch is reprocessed instead of recording an un-durable success.
+ if (syncReconDbLog()) {
+ for (ReconOmTask.TaskResult result : successfulTasks) {
+ String taskName = result.getTaskName();
+ // Track task delta processing success
+ taskMetrics.incrTaskDeltaProcessingSuccess(taskName);
+
+ ReconTaskStatusUpdater taskStatusUpdater =
+ taskStatusUpdaterManager.getTaskStatusUpdater(taskName);
+ taskStatusUpdater.setLastTaskRunStatus(0);
+ taskStatusUpdater.setLastUpdatedSeqNumber(events.getLastSequenceNumber());
+ taskStatusUpdater.recordRunCompletion();
+ }
+ } else {
+ for (ReconOmTask.TaskResult result : successfulTasks) {
+ String taskName = result.getTaskName();
+ failedTasks.add(new ReconOmTask.TaskResult.Builder()
+ .setTaskName(taskName)
+ .setSubTaskSeekPositions(result.getSubTaskSeekPositions())
+ .build());
+ ReconTaskStatusUpdater taskStatusUpdater =
+ taskStatusUpdaterManager.getTaskStatusUpdater(taskName);
+ taskStatusUpdater.setLastTaskRunStatus(-1);
+ taskStatusUpdater.recordRunCompletion();
+ }
+ }
+ }
+ }
+
+ /**
+ * Flushes and syncs the Recon derived-data RocksDB write-ahead log to stable storage so the
+ * derived writes for the processed batch are durable before the task-status cursor advances.
+ * Returns {@code false} if the sync fails, in which case the caller must not advance the cursor,
+ * so that the cursor is never persisted ahead of the derived data.
+ */
+ private boolean syncReconDbLog() {
+ DBStore dbStore = reconDBProvider.getDbStore();
+ if (dbStore == null) {
+ return true;
Review Comment:
nit: Should a null dbStore return false here? Treating it as synced advances the cursor without any durability guarantee, though I realize this is mostly unreachable in practice.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ReconTaskControllerImpl.java:
##########
@@ -502,6 +502,65 @@ private void processTasks(
} catch (CancellationException ce) {
LOG.error("Some tasks were cancelled with exception", ce);
}
+
+ failedTasks.addAll(executionFailedTasks);
+
+ if (!successfulTasks.isEmpty()) {
+ // Make the derived-table RocksDB writes for this batch durable before the task-status
+ // cursors are committed to Derby. The cursor rows are fsync-durable while the RocksDB writes
+ // are not synced by default, so without this barrier a power loss can leave the durable
+ // cursors ahead of the (lost) derived data. Startup reconciliation then sees the derived
+ // and delta cursors at the same sequence number and never reprocesses, permanently
+ // dropping the applied update. Coalescing the barrier to once per batch syncs all successful
+ // task writes together before any cursor advances. If the sync fails, leave the cursors
+ // unadvanced so the batch is reprocessed instead of recording an un-durable success.
+ if (syncReconDbLog()) {
+ for (ReconOmTask.TaskResult result : successfulTasks) {
+ String taskName = result.getTaskName();
+ // Track task delta processing success
+ taskMetrics.incrTaskDeltaProcessingSuccess(taskName);
+
+ ReconTaskStatusUpdater taskStatusUpdater =
+ taskStatusUpdaterManager.getTaskStatusUpdater(taskName);
+ taskStatusUpdater.setLastTaskRunStatus(0);
+ taskStatusUpdater.setLastUpdatedSeqNumber(events.getLastSequenceNumber());
+ taskStatusUpdater.recordRunCompletion();
+ }
+ } else {
+ for (ReconOmTask.TaskResult result : successfulTasks) {
+ String taskName = result.getTaskName();
+ failedTasks.add(new ReconOmTask.TaskResult.Builder()
+ .setTaskName(taskName)
+ .setSubTaskSeekPositions(result.getSubTaskSeekPositions())
+ .build());
+ ReconTaskStatusUpdater taskStatusUpdater =
+ taskStatusUpdaterManager.getTaskStatusUpdater(taskName);
+ taskStatusUpdater.setLastTaskRunStatus(-1);
Review Comment:
nit: Would it be worth counting this as a failure in taskMetrics too? Right now a sync failure is invisible to anyone alerting on the delta metrics.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16463. Sync Recon derived DB before cursor advance [ozone]
Posted by "HesandaLiyanage (via GitHub)" <gi...@apache.org>.
HesandaLiyanage commented on PR #11262:
URL: https://github.com/apache/ozone/pull/11262#issuecomment-5771534748
@ivandika3 @chihsuan ping
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org