You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "merlimat (via GitHub)" <gi...@apache.org> on 2026/10/07 22:36:20 UTC
[PR] [fix][broker] Prune sealed scalable-topic segments ancestors first [pulsar]
merlimat opened a new pull request, #26842:
URL: https://github.com/apache/pulsar/pull/26842
### Motivation
**Problem.** The scalable-topic controller's sealed-segment GC prunes every sealed segment that is past
its retention and drained by every subscription, in any order. `SegmentLayout.pruneSegment` drops the
pruned segment from its children's parent lists without linking them to its own parents. Consumers treat
a parent that is missing from the layout as drained: the controller's parent-drain check for stream
subscriptions (`SubscriptionCoordinator.isAssignable`) and the ungrouped V5 checkpoint consumer in #26837
both rely on it. So pruning an *intermediate* segment while its parent is still in the DAG cuts its
children's link to that ancestor, and they can be read before it — breaking per-key order across the
split.
An intermediate segment is drained for every subscription as soon as it holds nothing, e.g. when it was
split again before any message was written to it: the split created each subscription's cursor on it at
the start, so its backlog is 0.
Found by @lhotari while reviewing #26837.
**Example.** Segment 0 splits into 1 and 2, then 2 splits into 3 and 4 before anything was written to 2.
Subscription `sub-a` is behind and still has backlog on 0.
- After retention, the GC tick sees 0 (backlog, not drained) and 2 (empty, drained) as candidates, and
prunes 2.
- The new layout lists 3 and 4 with no parents, so `isAssignable` hands them to `sub-a`'s consumers while
0 still holds older messages of their keys.
`ScalableTopicController.runGcTickAsync` today:
```java
for (SegmentInfo seg : layout.getAllSegments().values()) {
if (seg.isSealed() && seg.sealedAtMs() > 0
&& (now - seg.sealedAtMs()) >= retentionMs) {
candidates.add(seg);
}
}
```
### Modifications
**Change.** Only segments with no parent left in the DAG are GC candidates, so ancestors are pruned first,
one generation per tick:
```java
if (seg.isSealed() && seg.sealedAtMs() > 0
&& (now - seg.sealedAtMs()) >= retentionMs
&& layout.getParents(seg.segmentId()).isEmpty()) {
candidates.add(seg);
}
```
The Javadoc of `pruneEligibleAsync` said pruning was allowed in any order, and the one on
`SegmentLayout.pruneSegment` described a precondition the GC never applied; both now state the
ancestors-first rule. The DAG edges themselves are unchanged, so the merge-depth count and the auto-scale
cooldown seeding, which classify segments by their parents, are not affected.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change added tests and can be verified as follows:
- `ScalableTopicControllerTest.testGcTickPrunesAncestorsFirst`: with backlog left on segment 0, its drained
intermediate child is kept; once 0 drains it is pruned, and the child goes on the next tick. Without the
fix the child is pruned on the first tick.
- `ScalableTopicControllerTest`, `SegmentLayoutTest`, `SubscriptionCoordinatorTest`,
`V5SegmentReconciliationTest`, `V5CheckpointConsumerSkipDeletedSegmentTest` and
`V5StreamConsumerDagReplayTest` pass.
### 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][broker] Prune sealed scalable-topic segments ancestors first [pulsar]
Posted by "void-ptr974 (via GitHub)" <gi...@apache.org>.
void-ptr974 commented on PR #26842:
URL: https://github.com/apache/pulsar/pull/26842#issuecomment-6082224778
Thanks for the fix. One related case may be worth checking in `SubscriptionCoordinator.isAssignable()`.
With `0 → 2 → 3`, segment 0 still has unread messages for a key, segment 2 is sealed and empty, and segment 3 contains newer messages for the same key.
After 2 is marked as drained, the direct-parent check for 3 appears to be satisfied. It would be helpful to confirm how the assignment accounts for 0’s remaining backlog in this case.
--
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][broker] Prune sealed scalable-topic segments ancestors first [pulsar]
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui merged PR #26842:
URL: https://github.com/apache/pulsar/pull/26842
--
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][broker] Prune sealed scalable-topic segments ancestors first [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on PR #26842:
URL: https://github.com/apache/pulsar/pull/26842#issuecomment-6099567120
Thanks, good catch: it doesn't. `isAssignable` only checks an active segment's direct parents, and an empty intermediate segment counts as drained as soon as the drain poll sees its zero backlog, so 3 is assigned while 0 still has backlog. It's wider than that, too: a sealed segment wasn't checked against its parents at all, so if 2 does hold messages, the subscription reads 0 and 2 at the same time.
That's separate from the prune order fixed here, so I opened #26850 for it: a segment, sealed or active, is assigned only once every ancestor still in the layout is drained for the subscription. It relies on this PR for ancestors that were pruned.
--
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