#4872 Fix single-replica ledger autorecovery loop - #4873
Open
Radiancebobo wants to merge 1 commit into
Open
Conversation
Skip failed-bookie recovery tasks for ledgers with writeQuorumSize=1, and clean up historical tasks without affecting placement-policy repairs. Add dedicated metrics and regression tests for task filtering, lock release, normal recovery retries, and cleanup failures.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
This PR prevents futile autorecovery loops for ledgers with writeQuorumSize=1 by skipping failed-bookie underreplication tasks (and cleaning up historical ones), while adding dedicated operational metrics and regression tests.
Changes:
- Auditor: read ledger metadata before publishing underreplication tasks and skip failed-bookie tasks for
writeQuorumSize==1ledgers while keeping fail-open behavior on metadata errors. - ReplicationWorker: detect and clean up historical single-replica failed-bookie tasks without attempting replication, tracking a dedicated “skipped” outcome metric.
- Tests & docs: add regression tests for the new filtering/cleanup behavior and document the operational meaning of the new counters.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| site3/website/docs/admin/autorecovery.md | Documents skipped single-replica failed-bookie tasks and the new metrics. |
| bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestReplicationWorker.java | Adds worker regression tests for historical cleanup, placement-policy preservation, and metrics. |
| bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AuditorBookieCheckTaskTest.java | Adds auditor regression tests for single-replica filtering, deleted ledgers, and fail-open behavior. |
| bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/ReplicationWorker.java | Introduces “skipped” outcome handling and historical cleanup guard for single-replica tasks. |
| bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/ReplicationStats.java | Adds new metric constants for auditor and worker skip counters. |
| bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/AuditorTask.java | Implements metadata-based filtering and adjusts published-ledger stats to exclude skipped items. |
| bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/AuditorStats.java | Adds auditor counter for single-replica skip events. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+93
to
+127
| CompletableFuture<List<Void>> publishFuture = FutureUtils.processList( | ||
| Lists.newArrayList(ledgers), | ||
| ledgerId -> | ||
| ledgerManager.readLedgerMetadata(ledgerId).whenComplete((metadata, exception) -> { | ||
| if (exception == null) { | ||
| underReplicatedSize.add(metadata.getValue().getLength()); | ||
| } | ||
| }), null).whenComplete((res, e) -> { | ||
| ledgerId -> ledgerManager.readLedgerMetadata(ledgerId).handle((metadata, exception) -> { | ||
| if (exception != null) { | ||
| if (BKException.getExceptionCode(exception) | ||
| == BKException.Code.NoSuchLedgerExistsOnMetadataServerException) { | ||
| log.info() | ||
| .attr("ledgerId", ledgerId) | ||
| .log("Ledger was deleted before publishing underreplicated mark"); | ||
| return FutureUtils.Void(); | ||
| } | ||
| log.warn() | ||
| .attr("ledgerId", ledgerId) | ||
| .exception(exception) | ||
| .log("Unable to read ledger metadata; publishing underreplicated mark fail-open"); | ||
| } else if (metadata == null || metadata.getValue() == null) { | ||
| log.warn() | ||
| .attr("ledgerId", ledgerId) | ||
| .log("Ledger metadata was empty; publishing underreplicated mark fail-open"); | ||
| } else if (metadata.getValue().getWriteQuorumSize() == 1) { | ||
| auditorStats.getNumSingleReplicaLedgersSkipped().inc(); | ||
| log.info() | ||
| .attr("ledgerId", ledgerId) | ||
| .attr("writeQuorumSize", metadata.getValue().getWriteQuorumSize()) | ||
| .attr("reason", "single-replica-ledger") | ||
| .attr("action", "skip-publish") | ||
| .log("Skipping underreplicated mark"); | ||
| return FutureUtils.Void(); | ||
| } else { | ||
| underReplicatedSize.add(metadata.getValue().getLength()); | ||
| } | ||
|
|
||
| publishedLedgers.increment(); | ||
| return ledgerUnderreplicationManager.markLedgerUnderreplicatedAsync(ledgerId, missingBookies); | ||
| }).thenCompose(markFuture -> markFuture), null).whenComplete((res, e) -> { |
|
|
||
| The auditor does not publish failed-bookie tasks for ledgers with `writeQuorumSize == 1`. Each entry in such a ledger has only one data source, so if that source Bookie is permanently lost, AutoRecovery cannot reconstruct the data. The replication worker also removes matching historical failed-bookie tasks created before an upgrade. This behavior does not apply to placement-policy repair tasks, which have no failed Bookie in their replica list and may still migrate data from an available source. | ||
|
|
||
| Skipped ledgers are reported by the `NUM_SINGLE_REPLICA_LEDGERS_SKIPPED` Auditor counter and the `NUM_SINGLE_REPLICA_UNDERREPLICATED_LEDGERS_SKIPPED` replication-worker counter. These counters indicate possible data loss, not successful recovery. Operators should alert on them and use the original Bookie disk, backups, or application-specific recovery procedures when the data must be restored. |
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.
Skip failed-bookie recovery tasks for ledgers with writeQuorumSize=1, and clean up historical tasks without affecting placement-policy repairs.
Add dedicated metrics and regression tests for task filtering, lock release, normal recovery retries, and cleanup failures.
Fix #4872
Motivation
BookKeeper can create ledgers with
writeQuorumSize=1. Every entry in such aledger has only one data source. If that Bookie is permanently lost, the
Auditor currently publishes an underreplication task, and the
ReplicationWorkerrepeatedly tries to read a source replica that cannotexist. The retry loop consumes worker, client, and ZooKeeper resources and
creates noisy failure logs while obscuring ledgers that can actually be
recovered.
This change intentionally does not claim to recover or reconstruct lost data.
It stops an impossible automatic recovery path and exposes a dedicated metric
so operators can alert on the skipped condition and use backups or manual
recovery when data must be restored.
Changes
AuditorTask.publishSuspectedLedgersAsync:writeQuorumSize == 1.marked for normal recovery.
manager.
NUM_SINGLE_REPLICA_LEDGERS_SKIPPEDin the Auditor scope and log theledger id, quorum size, reason, and action for skipped publications.
ReplicationWorkerfor historical tasks thatpredate this change. After taking the task lock, a task is removed through
markLedgerReplicatedwhenwriteQuorumSize == 1and itsreplicaListisnon-empty (the failed-Bookie task shape).
writeQuorumSize == 1task withan empty
replicaListcontinues through the existing replication path.SUCCESS,FAILED, andSKIPPEDworker outcomes separately sohistorical cleanup is not counted as replication success or failure. Add
NUM_SINGLE_REPLICA_UNDERREPLICATED_LEDGERS_SKIPPEDfor this path.ledgers, fail-open metadata errors, published-ledger statistics, historical
cleanup, placement-policy task preservation, and skipped-operation metrics.
Compatibility and operational notes
No configuration, ZooKeeper schema, protobuf field, or public API is added.
The behavior is deterministic from ledger metadata and applies on upgrade.
Existing underreplication znodes are persistent, so the Worker-side guard is
required to converge historical failed-Bookie tasks. Tasks with an empty
replicaList, unreadable metadata, or a concurrent znode version change arenot forcibly removed by the single-replica cleanup guard. If metadata is
unreadable, the Worker falls back to the normal recovery path; that path may
remove the task if recovery completes successfully.
The skipped counters are operational signals, not recovery-success counters:
auditor.NUM_SINGLE_REPLICA_LEDGERS_SKIPPEDreplication_worker.NUM_SINGLE_REPLICA_UNDERREPLICATED_LEDGERS_SKIPPEDThese counters are cumulative skip events, not a deduplicated count of unique
ledger IDs. The same ledger may be counted again during a later audit or retry.
Verification
Tests run on the branch:
The command passes with 25 tests: 6 Auditor tests and 19 ReplicationWorker
tests. The full BookKeeper precommit command remains appropriate before merge:
mvn clean apache-rat:check install spotbugs:check.