Skip to content

[fix][broker] Restore PendingAcksMap#get binary compatibility#26180

Open
Denovo1998 wants to merge 1 commit into
apache:masterfrom
Denovo1998:restore_pendingAcksMap_get_binary_compatibility
Open

[fix][broker] Restore PendingAcksMap#get binary compatibility#26180
Denovo1998 wants to merge 1 commit into
apache:masterfrom
Denovo1998:restore_pendingAcksMap_get_binary_compatibility

Conversation

@Denovo1998

Copy link
Copy Markdown
Contributor

Motivation

PR #26030 replaced PendingAcksMap object values with packed primitive values and removed the public PendingAcksMap#get(long, long) method. However, this method was included in previous Pulsar releases, including Pulsar 4.0.x and 4.1.x, and is reachable by broker extensions through the public Consumer#getPendingAcks() method.

A BrokerInterceptor compiled against one of those releases can therefore fail with NoSuchMethodError after the broker is upgraded to a version containing #26030.

Why NoSuchMethodError occurs

BrokerInterceptor callbacks such as messageDispatched receive a broker Consumer instance. An existing extension can use the following API path:

@Override
public void messageDispatched(ServerCnx cnx, Consumer consumer,
                              long ledgerId, long entryId,
                              ByteBuf headersAndPayload) {
    IntIntPair pendingAck =
            consumer.getPendingAcks().get(ledgerId, entryId);
}

When this extension is compiled against an earlier Pulsar release, the Java compiler records the following symbolic method reference in the extension bytecode:

org/apache/pulsar/broker/service/PendingAcksMap.get:
    (JJ)Lit/unimi/dsi/fastutil/ints/IntIntPair;

The relevant bytecode is equivalent to:

invokevirtual Consumer.getPendingAcks:
    ()Lorg/apache/pulsar/broker/service/PendingAcksMap;

invokevirtual PendingAcksMap.get:
    (JJ)Lit/unimi/dsi/fastutil/ints/IntIntPair;

The extension does not get recompiled when the broker is upgraded. When the interceptor callback is executed, the JVM resolves the existing invokevirtual reference against the PendingAcksMap loaded from the upgraded broker.

After #26030, that exact method name and descriptor no longer exist. The newly added primitive accessor has a different name and return type:

PendingAcksMap.getRemainingUnacked:(JJ)I

The JVM cannot substitute one method for the other. Method resolution therefore fails with a linkage error similar to:

java.lang.NoSuchMethodError:
  'it.unimi.dsi.fastutil.ints.IntIntPair
   org.apache.pulsar.broker.service.PendingAcksMap.get(long, long)'

The interceptor can load successfully and only fail when the affected callback and call site are first executed, making the problem dependent on extension behavior and broker traffic.

Modifications

  • Restore the exact PendingAcksMap#get(long, long) JVM method signature available in previous releases.
  • Mark the restored method as deprecated so new broker code continues to prefer primitive accessors.
  • Reconstruct IntIntPair from the packed primitive value while holding the existing read lock.
  • Return null for missing ledger and entry mappings, preserving the previous behavior.
  • Add coverage for maximum remaining-unacked values, signed sticky-key hashes, and missing entries.
  • Add a BrokerInterceptor extension fixture that accesses the method through Consumer#getPendingAcks(), reproducing the extension API call path.

Performance impact

The broker production paths changed by #26030 continue to use getRemainingUnacked and removeAndGetRemainingUnacked. They do not call the compatibility method and therefore do not recreate IntIntPair objects on the pending-ack hot paths.

An IntIntPair is allocated only when an existing extension explicitly invokes the deprecated compatibility method.

Verifying this change

  • Make sure that the change passes the CI checks.

The following targeted test and style checks pass:

./gradlew :pulsar-broker:test \
  --tests org.apache.pulsar.broker.service.PendingAcksMapTest \
  --max-workers=1 \
  -PtestRetryCount=0 \
  :pulsar-broker:checkstyleMain \
  :pulsar-broker:checkstyleTest

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

@void-ptr974

Copy link
Copy Markdown
Contributor

Could you clarify the concrete external use case that requires PendingAcksMap#get?

PendingAcksMap and this method expose broker dispatcher implementation state; they are not a supported user-facing API or a broker-extension compatibility contract. After #26030, broker code uses the primitive accessors and no longer needs this pair-returning API.

Restoring it only to preserve a hypothetical call site would add an internal representation, including the fastutil IntIntPair type, to the compatibility surface and turn obsolete implementation detail into long-term maintenance debt. Unless there is a concrete supported use case, I do not think we should restore this method. If an extension genuinely needs this information, we should define a dedicated supported API rather than expose PendingAcksMap directly.

@Denovo1998

Copy link
Copy Markdown
Contributor Author

@void-ptr974 I think you are right too.
@lhotari Hi, lari. WDYT?

@lhotari

lhotari commented Jul 13, 2026

Copy link
Copy Markdown
Member

Hi, lari. WDYT?

@Denovo1998 Similar thoughts as @void-ptr974 expressed in #26180 (comment). It would be great to hear what the use case is.

@Denovo1998

Copy link
Copy Markdown
Contributor Author

@lhotari
Oh, I was just reviewing the PRs that have been merged every week and found potential issues.

Although MoP uses getPendingAcks().remove(...), which indicates that someone is indeed accessing this internal state, it may not prove that the removed get has an actual use case.

https://github.com/streamnative/mop/blob/edd40621925c08eab0c910fb1f84fc9b6501fae7/mqtt-broker/src/main/java/io/streamnative/pulsar/handlers/mqtt/broker/processor/MQTTBrokerProtocolMethodProcessor.java#L167-L192

So I think it's okay to close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants