[fix][ml] Preserve ledger properties when recovering the last ledger on open

[dao-jun]

Motivation

ManagedLedgerImpl#initialize() unconditionally re-opens the last ledger on every
managed-ledger open to refresh its stats from BookKeeper, because the znode stat of the
ledger that was the current writing ledger is stale (it is only persisted on ledger
rollover). However, the rebuild constructed the LedgerInfo from scratch:

LedgerInfo info = new LedgerInfo().setLedgerId(id)
        .setEntries(lh.getLastAddConfirmed() + 1).setSize(lh.getLength())
        .setTimestamp(clock.millis());
ledgers.put(id, info);

dropping every other field already persisted in the metadata store — per-ledger properties and the offload context. The loss is then made durable by the full ledger-ids rewrite in initializeBookKeeper() ("Save it back to ensure all nodes exist"), silently, with no error log.

This breaks the persistence contract of asyncAddLedgerProperty / asyncRemoveLedgerProperty: a ledger property only survives until the managed ledger is next opened (topic load, unload/reload, broker restart), then disappears, while the APIs report success all along. It also wipes the offload context of the last ledger, so an already-offloaded ledger looks like it was never offloaded after a reopen, which allows redundant offloads and skips the bookkeeper-deletion bookkeeping during trimming.

The same merge pattern (keep the persisted fields, refresh only entries/size/timestamp) is already used by ledgerClosed() on the rollover path; the recovery path in initialize() is the only remaining place that rebuilds a LedgerInfo of an existing ledger from scratch.

Modifications

In ManagedLedgerImpl#initialize(), replace the from-scratch LedgerInfo rebuild of the last ledger with the same merge already used by ledgerClosed(): the new info is copyFrom(oldInfo), keeping every persisted field (properties, offload context, ...), and only entries / size / timestamp are overridden with the authoritative BookKeeper values. The Terminated, no-such-ledger and empty-ledger cleanup paths are unchanged.

Verifying this change

• Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

• Added integration tests for end-to-end deployment with large payloads (10MB)
• Extended integration test for recovery after broker failure

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

[lhotari]

@dao-jun I added "Fixes #26483" to the description. I assume that's the issue that this PR is fixing.

[dao-jun]

@lhotari
Thanks! But this PR doesn't actually fix #26483 — different bug, same family.

This PR fixes ManagedLedgerImpl#initialize() dropping per-ledger properties and the
offload context (ManagedLedgerInfo.LedgerInfo) when refreshing the last ledger's
stats on open. #26483 is ManagedCursorImpl#recoverFromLedger() wiping cursor
properties (ManagedCursorInfo.properties) via Collections.emptyMap() — no cursor
changes in this PR, so your suggested fix is still needed.

I'll delete "Fixes #26483"

[Copilot]

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

Fixes managed-ledger reopen recovery so the last ledger’s persisted metadata (ledger properties + offload context) is preserved while refreshing authoritative stats from BookKeeper.

Changes:

• Update ManagedLedgerImpl#initialize() to merge refreshed BK stats into existing LedgerInfo instead of rebuilding it from scratch.
• Add regression tests that validate ledger properties and offload context survive reopen, and that the no-properties case remains unchanged.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java	Preserves existing LedgerInfo fields on reopen by copying old info and only overriding entries/size/timestamp from BK.
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerTest.java	Adds tests to prevent regression for ledger properties + offload context across reopen.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[void-ptr974]

Reviewed the metadata recovery change and focused regression coverage. The implementation preserves existing LedgerInfo fields while refreshing BookKeeper-derived stats, and the targeted tests pass locally. No blocking findings.

[lhotari]

This fix is also relevant to branch-4.0, even though that branch does not contain PIP-404 per-ledger properties. Its LedgerInfo already contains offloadContext, which is also discarded when reopening a managed ledger rebuilds the last ledger's metadata from BookKeeper. Copying the existing metadata before refreshing entries, size, and timestamp preserves that context too.

The local branch-4.0 backport uses the older protobuf builder API (oldInfo.toBuilder() and build()). The adapted regression tests verify that reopening preserves persisted offload context while refreshing entry count and size, and that reopening a ledger without offload context does not introduce one. Compilation and the focused regression tests passed.