Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C++] Avoid sending flow requests with zero permits #10506

Merged
merged 2 commits into from
May 7, 2021

Conversation

BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented May 7, 2021

Motivation

When broker receives a FLOW request with zero permit, an IllegalArgumentException will be thrown in ServerCnx#handleFlow and then the connection will be closed so that the consumer will reconnect to the broker. If resumeMessageListener() was called for a zero queue consumer, the permits may be zero and trigger a reconnection. The frequent reconnections may cause messages repeated or out of order.

Modifications

  • Validate the permits of a FLOW command before sending it.
  • Add a test that a zero queue consumer resumes and pauses for each message periodically. Before this PR, there's a great chance that the received messages are repeated or out of order, even a ACK was sent.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests ZeroQueueSizeTest#testPauseResumeNoReconnection.

@BewareMyPower
Copy link
Contributor Author

In addition to the PR description, when a zero queue consumer resumes, the broker's logs will be

22:12:52.953 [pulsar-io-52-21] WARN  org.apache.pulsar.broker.service.ServerCnx - [/127.0.0.1:58677] Got exception java.lang.IllegalArgumentException
        at com.google.common.base.Preconditions.checkArgument(Preconditions.java:128)
        at org.apache.pulsar.broker.service.Consumer.flowPermits(Consumer.java:524)
        at org.apache.pulsar.broker.service.ServerCnx.handleFlow(ServerCnx.java:1423)
        at org.apache.pulsar.common.protocol.PulsarDecoder.channelRead(PulsarDecoder.java:181)

@merlimat merlimat merged commit 37583f6 into apache:master May 7, 2021
@BewareMyPower BewareMyPower deleted the bewaremypower/cpp-flow-zero branch May 10, 2021 01:15
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request May 11, 2021
* Avoid sending zero permits

* Avoid the same topic name
@codelipenghui codelipenghui added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label May 13, 2021
codelipenghui pushed a commit that referenced this pull request May 13, 2021
* Avoid sending zero permits

* Avoid the same topic name

(cherry picked from commit 37583f6)
eolivelli pushed a commit to datastax/pulsar that referenced this pull request May 21, 2021
* Avoid sending zero permits

* Avoid the same topic name

(cherry picked from commit 37583f6)
@Anonymitaet
Copy link
Member

@BewareMyPower Clear PR description! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/branch-2.7 Archived: 2.7 is end of life release/2.7.3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants