You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "lhotari (via GitHub)" <gi...@apache.org> on 2026/06/13 19:05:31 UTC
[PR] [fix][broker] Fix forced topic/namespace deletion still hanging when the compaction reader reconnect stalls [pulsar]
lhotari opened a new pull request, #26026:
URL: https://github.com/apache/pulsar/pull/26026
Follow-up to #26016 (which fixed #24148, "Compaction of `__change_events` topic is blocking forceful
namespace/topic deletion"). This addresses a residual race that #26016 left behind.
### Motivation
#26016 fixed two causes of forced topic/namespace deletion hanging while compaction is in progress
(the poisoned `currentCompaction` cursor deletion, and the `receiveRawAsync` enqueue-vs-close race),
but `CompactionTest#testForcedNamespaceDeleteWithInflightCompaction` still failed intermittently
(~50%).
The compaction `RawReader`'s consumer (`RawReaderImpl.RawConsumerImpl`, created with
`retryOnRecoverableErrors=false`) has an asymmetry between its two reconnect-failure paths. When a
forced delete fences the topic and disconnects the consumer, the client reconnects and the failure
surfaces at one of two stages:
- **Subscribe stage** (`ConsumerImpl.connectionOpened().exceptionally`) consults
`isUnrecoverableError` unconditionally, so a `ServiceNotReadyException` closes the reader promptly
and fails the in-flight read.
- **Lookup / connection stage** (`ConsumerImpl.connectionFailed`) only consults `isUnrecoverableError`
inside `if (nonRetriableError || timeout)`. For a *retriable* `ServiceNotReadyException` within the
lookup deadline it just reconnects — so the in-flight read never fails, `currentCompaction` never
completes, and the forced deletion blocks past the test's 20s budget.
Which stage wins is a metadata-cache propagation race, hence the flakiness. (This is independent of
#26009, which only stops redirect-URL pinning across reconnects and does not change this
`connectionFailed` classification.)
### Modifications
- `RawReaderImpl.RawConsumerImpl` overrides `connectionFailed` to honor `isUnrecoverableError` before
the retriable / lookup-deadline gate, so a fenced/deleted-topic reconnect terminates the reader
promptly regardless of which stage surfaces the failure (mirroring the subscribe-stage behavior).
This only affects the compaction reader (`retryOnRecoverableErrors=false`); ordinary RawReaders keep
retrying recoverable errors unchanged.
- `PersistentTopic.asyncDeleteCursorWithCleanCompactionLedger` skips waiting on `currentCompaction`
when the topic is already closing/deleting. A forced delete aborts the in-flight compaction, so the
cursor deletion must not block on a compaction future that may stay pending while its reader keeps
reconnecting. This is the deterministic guarantee, independent of client reconnect timing or error
classification.
### Verifying this change
This change added tests and can be verified as follows:
- `CompactionTest#testForcedDeleteCompletesWhileCompactionStuck`: blocks the compaction at the
phase-two seek so `currentCompaction` never completes, then asserts `deleteForcefully()` still
completes promptly (deterministic — exercises the broker-side gate).
- `RawReaderTest#testConnectionFailureTerminatesReadWhenNotRetryingRecoverableErrors`: builds a
`retryOnRecoverableErrors=false` consumer with a pending read, invokes
`connectionFailed(ServiceNotReadyException)`, and asserts it terminates the reader and fails the read
(deterministic — exercises the client-side override).
- `testForcedNamespaceDeleteWithInflightCompaction` passed 10/10 under repetition with the fix; the
full `CompactionTest` and `RawReaderTest` suites pass.
### Does this pull request potentially affect one of the following parts:
*NONE of these are affected with this change.*
- [ ] 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] Fix forced topic/namespace deletion still hanging when the compaction reader reconnect stalls [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari merged PR #26026:
URL: https://github.com/apache/pulsar/pull/26026
--
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] Fix forced topic/namespace deletion still hanging when the compaction reader reconnect stalls [pulsar]
Posted by "void-ptr974 (via GitHub)" <gi...@apache.org>.
void-ptr974 commented on code in PR #26026:
URL: https://github.com/apache/pulsar/pull/26026#discussion_r3409245316
##########
pulsar-broker/src/main/java/org/apache/pulsar/client/impl/RawReaderImpl.java:
##########
@@ -160,6 +161,25 @@ protected boolean isUnrecoverableError(Throwable t) {
return super.isUnrecoverableError(t);
}
+ @Override
+ public boolean connectionFailed(PulsarClientException exception) {
+ // A compaction reader is created with retryOnRecoverableErrors=false. When the topic is fenced or
+ // deleted, a reconnect can fail at the lookup/connection stage with a retriable error such as
+ // ServiceNotReadyException. The base ConsumerImpl.connectionFailed only consults isUnrecoverableError
+ // for non-retriable errors (or after the lookup deadline has passed), so for such a retriable error it
+ // would keep reconnecting and never fail the in-flight read, leaving the compaction future pending.
+ // Honor isUnrecoverableError here too so the reader is closed promptly and pending reads are failed,
+ // mirroring the subscribe-stage handling in ConsumerImpl.connectionOpened(). This matters for
+ // compaction: a never-failing read keeps the compaction future pending, which blocks forced
+ // topic/namespace deletion (issue #24148).
+ Throwable actError = FutureUtil.unwrapCompletionException(exception);
+ if (isUnrecoverableError(actError)) {
+ closeWhenReceivedUnrecoverableError(actError, null);
+ return false;
Review Comment:
I found a possible case where `ServiceNotReadyException` is triggered before the initial raw-reader subscribe completes. In that case, this override may return `false` and close the consumer without completing `subscribeFuture` exceptionally, so `RawReader.create(...)` could remain pending. It may be worth covering this pre-subscribe failure case as well.
--
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] Fix forced topic/namespace deletion still hanging when the compaction reader reconnect stalls [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #26026:
URL: https://github.com/apache/pulsar/pull/26026#discussion_r3409386691
##########
pulsar-broker/src/main/java/org/apache/pulsar/client/impl/RawReaderImpl.java:
##########
@@ -160,6 +161,25 @@ protected boolean isUnrecoverableError(Throwable t) {
return super.isUnrecoverableError(t);
}
+ @Override
+ public boolean connectionFailed(PulsarClientException exception) {
+ // A compaction reader is created with retryOnRecoverableErrors=false. When the topic is fenced or
+ // deleted, a reconnect can fail at the lookup/connection stage with a retriable error such as
+ // ServiceNotReadyException. The base ConsumerImpl.connectionFailed only consults isUnrecoverableError
+ // for non-retriable errors (or after the lookup deadline has passed), so for such a retriable error it
+ // would keep reconnecting and never fail the in-flight read, leaving the compaction future pending.
+ // Honor isUnrecoverableError here too so the reader is closed promptly and pending reads are failed,
+ // mirroring the subscribe-stage handling in ConsumerImpl.connectionOpened(). This matters for
+ // compaction: a never-failing read keeps the compaction future pending, which blocks forced
+ // topic/namespace deletion (issue #24148).
+ Throwable actError = FutureUtil.unwrapCompletionException(exception);
+ if (isUnrecoverableError(actError)) {
+ closeWhenReceivedUnrecoverableError(actError, null);
+ return false;
Review Comment:
Good catch, thanks. You're right — `closeWhenReceivedUnrecoverableError` only closed the consumer and failed in-flight reads; it never completed the subscribe future, so an unrecoverable error before the initial subscribe (at the lookup stage via `connectionFailed`, or the subscribe stage via `connectionOpened`) left `RawReader.create(...)` pending.
Fixed at the root by completing the subscribe future exceptionally in `closeWhenReceivedUnrecoverableError` — a no-op once it's already done, so it's a no-op for normal consumers (which only reach this path post-subscribe), and it covers both the `connectionFailed` case you flagged and the equivalent `connectionOpened` one. Added a `RawReaderTest` case that injects the failure while the consumer future is still pending and asserts it completes exceptionally.
Done in 3c4a3fa95d7.
--
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