You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "dao-jun (via GitHub)" <gi...@apache.org> on 2026/07/22 19:52:44 UTC
[PR] [fix][ml] Preserve ledger entries/size when transformLedgerInfo callback completes after a concurrent close [pulsar]
dao-jun opened a new pull request, #26228:
URL: https://github.com/apache/pulsar/pull/26228
### Motivation
tryTransformLedgerInfo (shared by per-ledger property add/remove and offload metadata updates) writes its result back to the in-memory ledgers map via a blind ledgers.put(ledgerId, newInfo), where newInfo carries entries/size/timestamp snapshotted at transform time. If the ledger fills and ledgerClosed updates those
fields during the async metadata-store round-trip, the callback overwrites them with the stale snapshot values. The regressed value is also captured by the next persisted metadata snapshot, so the wrong counts can survive a restart.
### Modifications
1. Replace the blind put with a merge: keep the transform's changed fields (properties, offload context, ledger id) from newInfo, take entries/size/timestamp from the current in-memory value.
2. Add tests
### 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:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*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
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][ml] Preserve ledger entries/size when transformLedgerInfo callback completes after a concurrent close [pulsar]
Posted by "dao-jun (via GitHub)" <gi...@apache.org>.
dao-jun commented on PR #26228:
URL: https://github.com/apache/pulsar/pull/26228#issuecomment-5065616970
> 1. **Pre-existing, out of scope for this PR** — the mirror direction of this race remains, and it's deterministic: `ledgerClosed` ([`ManagedLedgerImpl.java` L1972-L1974](https://github.com/apache/pulsar/blob/master/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L1972-L1974)) rebuilds the `LedgerInfo` from scratch (ledgerId/entries/size/timestamp only), dropping any PIP-404 per-ledger `properties` already present in the in-memory map. A property added to the open current ledger is silently lost when that ledger later closes normally — no race required. The new test only passes because in its ordering (close before callback) the merge re-applies the property on top of the rebuilt info; with the opposite (normal) ordering the property is gone. Worth a follow-up issue.
fixed by https://github.com/apache/pulsar/pull/26227
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][ml] Preserve ledger entries/size when transformLedgerInfo callback completes after a concurrent close [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #26228:
URL: https://github.com/apache/pulsar/pull/26228#discussion_r3638864008
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -3878,11 +3878,16 @@ private void tryTransformLedgerInfo(long ledgerId, LedgerInfoTransformation tran
final HashMap<Long, LedgerInfo> newLedgers = new HashMap<>(ledgers);
newLedgers.put(ledgerId, newInfo);
store.asyncUpdateLedgerIds(name, buildManagedLedgerInfo(newLedgers), ledgersStat,
- new MetaStoreCallback<Void>() {
+ new MetaStoreCallback<>() {
@Override
public void operationComplete(Void result, Stat stat) {
ledgersStat = stat;
- ledgers.put(ledgerId, newInfo);
+ ledgers.merge(ledgerId, newInfo, (existing, incoming) -> {
+ LedgerInfo merged = new LedgerInfo();
+ merged.copyFrom(incoming);
+ return merged.setEntries(existing.getEntries()).setSize(existing.getSize())
+ .setTimestamp(existing.getTimestamp());
+ });
Review Comment:
Two related suggestions on this merge, both cheap to address while this line is being rewritten anyway:
1. **Field presence**: lightproto numeric getters return `0` for unset optional fields, and the setters flip the presence bit. For a still-open ledger the in-memory info has `entries`/`size` unset (`new LedgerInfo().setLedgerId(..).setTimestamp(0)`), so the common no-race path — a property write completing on the current ledger — now converts them into explicitly-present zeros. I audited the in-tree `hasEntries()`/`hasSize()`/`hasTimestamp()` consumers and they all guard with `> 0` / `== 0`, so nothing breaks today (the only visible effect is the admin managed-ledger-info path reporting `entries: 0` instead of omitting the field, once the polluted in-memory info is persisted by a later ledger-list update). Still, preserving presence keeps the invariant intact for future consumers.
2. **Absent key**: `Map.merge` inserts `newInfo` when the key is absent. The pre-flight `oldInfo == null` check above handles removal before the transform starts, but `ledgerClosed` can also remove the entry mid-round-trip when the current ledger closes empty (L1977) — again without holding `metadataMutex` — and this callback would then resurrect a ledger that was just discarded. (Pre-existing with the old blind `put` as well.)
Something like this addresses both:
```java
ledgers.computeIfPresent(ledgerId, (id, existing) -> {
LedgerInfo merged = new LedgerInfo();
merged.copyFrom(newInfo);
merged.clearEntries();
merged.clearSize();
merged.clearTimestamp();
if (existing.hasEntries()) {
merged.setEntries(existing.getEntries());
}
if (existing.hasSize()) {
merged.setSize(existing.getSize());
}
if (existing.hasTimestamp()) {
merged.setTimestamp(existing.getTimestamp());
}
return merged;
});
```
##########
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerTest.java:
##########
@@ -4829,6 +4829,85 @@ public void testRemoveLedgerProperty() throws Exception {
Assert.assertEquals(ml.getLedgersInfo().get(lastLedger).getPropertiesCount(), 0);
}
+ /**
+ * Verifies that a ledger-property write whose metadata-store callback completes AFTER the ledger
+ * has been closed does not regress the closed ledger's entries.
+ *
+ * <p>The transform callback in {@code tryTransformLedgerInfo} previously did a blind
+ * {@code ledgers.put(ledgerId, newInfo)}, where {@code newInfo} carried the entry count captured at
+ * transform time (before the close). If the ledger filled and {@code ledgerClosed} updated the
+ * in-memory entry count in the meantime, the callback overwrote it with the stale pre-close value.
+ * The fix merges instead, keeping the transform's properties but taking entries/size/timestamp
+ * from the current in-memory value.
+ */
+ @Test
+ public void testLedgerPropertyWriteDoesNotRegressEntriesAfterConcurrentClose() throws Exception {
Review Comment:
Nice deterministic construction — the comment about not blocking inside the spy while the ML monitor is held is exactly right. A few coverage suggestions:
- also assert `size`/`timestamp` are preserved, not just `entries`;
- cover the no-concurrent-close path (property write completing on the still-open current ledger) — that's the path where the unconditional setters change lightproto field-presence semantics (see the comment on the merge);
- assert the corrected values actually reach the metadata store after the subsequent rollover persist (e.g. reopen the ML or read the store back), since surviving restart is the headline claim in the description.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][ml] Preserve ledger entries/size when transformLedgerInfo callback completes after a concurrent close [pulsar]
Posted by "dao-jun (via GitHub)" <gi...@apache.org>.
dao-jun merged PR #26228:
URL: https://github.com/apache/pulsar/pull/26228
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org