Skip to content

[fix][meta][branch-4.0] Tolerate concurrent creation of the underreplication LAYOUT node - #26248

Merged
nodece merged 1 commit into
apache:branch-4.0from
lhotari:lh-fix-underreplication-layout-badversion
Jul 27, 2026
Merged

[fix][meta][branch-4.0] Tolerate concurrent creation of the underreplication LAYOUT node#26248
nodece merged 1 commit into
apache:branch-4.0from
lhotari:lh-fix-underreplication-layout-badversion

Conversation

@lhotari

@lhotari lhotari commented Jul 26, 2026

Copy link
Copy Markdown
Member

Fixes: #16455

This PR targets branch-4.0 only. The same change is already on master
and branch-4.2, where it landed as an incidental hunk of #25219 (PIP-454)
rather than as a standalone fix — which is why it was never backported.
branch-4.1 and branch-3.0 still need it and will get separate PRs.

Motivation

PulsarLedgerUnderreplicationManager.checkLayout() creates
<ledgersRoot>/underreplication/LAYOUT with a check-then-act sequence: it tests
store.exists(layoutPath) and, when the node is absent, writes it with expected version
-1 (create-if-absent).

private void checkLayout() throws ReplicationException.CompatibilityException {
    while (true) {
        if (!store.exists(layoutPath).join()) {
            LedgerRereplicationLayoutFormat.Builder builder = LedgerRereplicationLayoutFormat.newBuilder();
            builder.setType(LAYOUT).setVersion(LAYOUT_VERSION);
            store.put(layoutPath, builder.build().toString().getBytes(UTF_8), Optional.of(-1L)).join();
        } else {
            ...

When several managers are constructed concurrently against the same metadata store, they
all observe the node as absent and all attempt the create. Exactly one wins; every loser
gets a BadVersionException, which the bare join() propagates out of the constructor
instead of retrying. The enclosing while (true) loop was clearly meant to handle this,
but it can never be reached.

This was reported in #16455 as a bookie failing to start when several bookies are
started at once:

java.util.concurrent.CompletionException:
  org.apache.pulsar.metadata.api.MetadataStoreException$BadVersionException:
  org.apache.zookeeper.KeeperException$BadVersionException:
  KeeperErrorCode = BadVersion for /ledgers/underreplication/LAYOUT
    at org.apache.pulsar.metadata.impl.ZKMetadataStore.handlePutResult(ZKMetadataStore.java:210)

The same race also makes AuditorPeriodicBookieCheckTest flaky on branch-4.0 — the
AuditorElector started in setUp() and the test's own LedgerUnderreplicationManager
race to create the node:

[ERROR] AuditorPeriodicBookieCheckTest.testPeriodicBookieCheckInterval  <<< FAILURE!
java.util.concurrent.ExecutionException:
  MetadataStoreException$BadVersionException: KeeperErrorCode = BadVersion
  for /ledgers/underreplication/LAYOUT

Modifications

Ignore BadVersionException from the create and let the enclosing while (true) loop
run again — the next iteration observes the node, validates the layout type and version,
and breaks. Any other failure is still rethrown.

try {
    store.put(layoutPath, builder.build().toString().getBytes(UTF_8), Optional.of(-1L)).get();
} catch (ExecutionException | InterruptedException e) {
    if (!(e.getCause() instanceof MetadataStoreException.BadVersionException)) {
        throw new RuntimeException(e);
    }
}

This is byte-for-byte the change master and branch-4.2 carry, apart from the
protobuf builder expression, which differs on branch-4.0 only because of the later
BookKeeper 4.18 API migration (#25886). Keeping it identical avoids divergence between
the branches.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as AuditorPeriodicBookieCheckTest
and LedgerUnderreplicationManagerTest, which exercise checkLayout() — though only
probabilistically, since the defect is a race.

To verify it deterministically I wrote a throwaway test (not included in this PR, to keep
the backport identical to master) that constructs 16 managers concurrently against one
store behind a CyclicBarrier, run 5× against both ZooKeeper and MockZooKeeper:

  • Before the fix — fails on the first concurrent round, reproducing the reported
    signature exactly:
    ExecutionException -> CompletionException -> MetadataStoreException$BadVersionException -> KeeperException$BadVersionException: KeeperErrorCode = BadVersion for /ledgers-.../underreplication/LAYOUT
  • After the fix — 10/10 invocations pass.

Existing tests on branch-4.0 with the fix applied:

Test Result
AuditorPeriodicBookieCheckTest 1 run, 0 failures
LedgerUnderreplicationManagerTest 105 run, 0 failures, 1 skipped
PulsarLayoutManagerTest 6 run, 0 failures

mvn checkstyle:check on pulsar-metadata: 0 violations.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

…ication LAYOUT node

### Motivation

`PulsarLedgerUnderreplicationManager.checkLayout()` creates
`<ledgersRoot>/underreplication/LAYOUT` with a check-then-act sequence: it tests
`store.exists(layoutPath)` and, when the node is absent, writes it with expected
version `-1` (create-if-absent). When several managers are constructed
concurrently against the same metadata store, they all observe the node as
absent and all attempt the create. Every loser receives a
`BadVersionException`, which the bare `join()` propagated out of the
constructor instead of retrying.

Reported in apache#16455 as a bookie failing to start when several bookies are
started at once:

    java.util.concurrent.CompletionException:
      MetadataStoreException$BadVersionException:
      KeeperException$BadVersionException: KeeperErrorCode = BadVersion
      for /ledgers/underreplication/LAYOUT
        at ZKMetadataStore.handlePutResult(ZKMetadataStore.java:210)

The same race makes `AuditorPeriodicBookieCheckTest` flaky on branch-4.0, where
an `AuditorElector` and the test's own `LedgerUnderreplicationManager` race to
create the node.

### Modifications

Ignore `BadVersionException` from the create and let the enclosing
`while (true)` loop run again: the next iteration observes the node, validates
the layout type and version, and breaks. Any other failure is still rethrown.

This ports the `checkLayout()` change already present on master and branch-4.2,
where it landed as part of apache#25219 (PIP-454) rather than as a standalone fix,
which is why it was never backported. branch-4.1 and branch-3.0 need the same
change.
@nodece
nodece merged commit 6d9648a into apache:branch-4.0 Jul 27, 2026
53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants