Copilot commented on code in PR #4873:
URL: https://github.com/apache/bookkeeper/pull/4873#discussion_r3974730230
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/AuditorTask.java:
##########
@@ -88,24 +88,47 @@ protected CompletableFuture<?>
publishSuspectedLedgersAsync(Collection<String> m
.attr("ledgers", ledgers)
.attr("missingBookies", missingBookies)
.log("Following ledgers are identified as underreplicated");
-
auditorStats.getNumUnderReplicatedLedger().registerSuccessfulValue(ledgers.size());
+ LongAdder publishedLedgers = new LongAdder();
LongAdder underReplicatedSize = new LongAdder();
- FutureUtils.processList(
+ 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) -> {
Review Comment:
`CompletableFuture.handle(...)` produces a value, but this lambda returns a
mix of `Void` (via `FutureUtils.Void()`) and `CompletableFuture<?>` (from
`markLedgerUnderreplicatedAsync`). That makes the `handle` result type
inconsistent and causes `.thenCompose(markFuture -> markFuture)` to receive a
non-`CompletionStage` in the skip/delete branches, which should fail to compile
(or throw at runtime if it compiles via raw types). Make the `handle` lambda
always return a `CompletableFuture<Void>` (e.g., return a completed future for
the skip/delete branches) and flatten with `thenCompose(Function.identity())`
(or equivalent) only when the lambda consistently returns futures.
##########
site3/website/docs/admin/autorecovery.md:
##########
@@ -102,7 +102,11 @@ Both of these components run as threads in the
[`AutoRecoveryMain`]({{site.javad
The auditor watches all bookies in the cluster that are registered with
ZooKeeper. Bookies register with ZooKeeper at startup. If the bookie crashes or
is killed, the bookie's registration in ZooKeeper disappears and the auditor is
notified of the change in the list of registered bookies.
-When the auditor sees that a bookie has disappeared, it immediately scans the
complete ledger list to find ledgers that have data stored on the failed
bookie. Once it has a list of ledgers for that bookie, the auditor will publish
a rereplication task for each ledger under the `/underreplicated/`
[znode](https://zookeeper.apache.org/doc/current/zookeeperOver.html) in
ZooKeeper.
+When the auditor sees that a bookie has disappeared, it immediately scans the
complete ledger list to find ledgers that have data stored on the failed
bookie. For ledgers with `writeQuorumSize > 1`, the auditor publishes a
rereplication task under the `/underreplicated/`
[znode](https://zookeeper.apache.org/doc/current/zookeeperOver.html) in
ZooKeeper.
+
+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.
Review Comment:
The doc names the counters without their full metric paths/scopes, which can
make them hard to locate in common stats backends. Consider documenting the
fully qualified names (e.g., `auditor.NUM_SINGLE_REPLICA_LEDGERS_SKIPPED` and
`replication_worker.NUM_SINGLE_REPLICA_UNDERREPLICATED_LEDGERS_SKIPPED`) to
match the operational notes in the PR description and reduce ambiguity for
operators.
--
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]
