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

[fix][broker] fix broker unackmessages become a negative number #17003

Conversation

congbobo184
Copy link
Contributor

@congbobo184 congbobo184 commented Aug 9, 2022

This PR fixes some cases where consumer unackedCount may become a negative number.

issue: reduce the consumer unackedCount use incorrect consumer.
reproduce step

        for (int i = 0; i < 5; i++) {
            producer.newMessage().value(("Hello Pulsar - " + i).getBytes()).sendAsync();
        }

        // consume-1 receive 5 batch messages
        List<MessageId> list = new ArrayList<>();
        for (int i = 0; i < 5; i++) {
            list.add(consumer1.receive().getMessageId());
        }

        // consumer-1 redeliver the batch messages
        consumer1.negativeAcknowledge(list.get(0));

        // consumer-2 will receive the messages that the consumer-1 redelivered
        for (int i = 0; i < 5; i++) {
            consumer2.receive().getMessageId();
        }

        // consumer1 ack two messages in the batch message
        consumer1.acknowledge(list.get(1));
        consumer1.acknowledge(list.get(2));

        // consumer-2 redeliver the rest of the messages
        consumer2.negativeAcknowledge(list.get(1));

        // consume-1 close will redeliver the rest messages to consumer-2
        consumer1.close();

        // consumer-2 can receive the rest of 3 messages
        for (int i = 0; i < 3; i++) {
            consumer2.acknowledge(consumer2.receive().getMessageId());
        }

        // consumer-2 can't receive any messages, all the messages in batch has been acked
        Message<byte[]> message = consumer2.receive(1, TimeUnit.SECONDS);
        assertNull(message);

        // the number of consumer-2's unacked messages is 0
        Awaitility.await().until(() -> getPulsar().getBrokerService().getTopic(topicName, false)
                .get().get().getSubscription(subName).getConsumers().get(0).getUnackedMessages() == 0);

in current code the getUnackedMessages = 2

Motivation

get the correct consumer and reduce the correct un acked messages

Modifications

change the method getAckedCountForBatchIndexLevelEnabled use ownerConsumer to check the pendingAck messages

Verifying this change

add the test

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

  • Does this pull request introduce a new feature? (yes)

  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

  • If a feature is not applicable for documentation, explain why?

  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

  • doc-not-needed

@congbobo184 congbobo184 added type/bug The PR fixed a bug or issue reported a bug area/broker doc-not-needed Your PR changes do not impact docs release/2.10.2 release/2.9.4 labels Aug 9, 2022
@congbobo184 congbobo184 added this to the 2.12.0 milestone Aug 9, 2022
@congbobo184 congbobo184 self-assigned this Aug 9, 2022
@Technoboy- Technoboy- modified the milestones: 2.12.0, 2.11.0 Aug 9, 2022
@congbobo184 congbobo184 added doc-not-needed Your PR changes do not impact docs and removed doc-not-needed Your PR changes do not impact docs labels Aug 9, 2022
@codelipenghui codelipenghui merged commit 5262e6c into apache:master Aug 13, 2022
Technoboy- pushed a commit to Technoboy-/pulsar that referenced this pull request Aug 14, 2022
@lhotari
Copy link
Member

lhotari commented Aug 15, 2022

@congbobo184 Would it be possible to describe the impact of the bug that was fixed by this PR? The current description describes implementation level details of the issue. However, most Pulsar users don't understand the implementation level, and therefore it would be useful if the externally observable behavior could be explained.
What externally observable bug was fixed by this PR?

Technoboy- pushed a commit to merlimat/pulsar that referenced this pull request Aug 16, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Aug 18, 2022
@mattisonchao
Copy link
Member

Hi, @congbobo184
Would you like to help cherry-pick this PR to branch-2.9?

congbobo184 added a commit that referenced this pull request Aug 30, 2022
@congbobo184 congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Aug 30, 2022
Jason918 pushed a commit that referenced this pull request Sep 4, 2022
@congbobo184
Copy link
Contributor Author

congbobo184 commented Sep 7, 2022

@congbobo184 Would it be possible to describe the impact of the bug that was fixed by this PR? The current description describes implementation level details of the issue. However, most Pulsar users don't understand the implementation level, and therefore it would be useful if the externally observable behavior could be explained. What externally observable bug was fixed by this PR?

@lhotari This PR fixes some cases where consumer unackedCount may become a negative number.

@congbobo184 congbobo184 changed the title [fix][broker] fix broker unackmessages number reduce error [fix][broker] fix broker unackmessages become a negative number Sep 7, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Sep 16, 2022
)

(cherry picked from commit 5262e6c)
(cherry picked from commit fd8e535)
@shibd shibd modified the milestones: 2.11.0, 3.0.0 Oct 25, 2023
shibd pushed a commit to shibd/pulsar that referenced this pull request Oct 25, 2023
@poorbarcode
Copy link
Contributor

Just explain the issue that the current PR fixed

  • the issue may lead unackedMessages of the consumer being larger than the correct value.

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

Successfully merging this pull request may close these issues.

10 participants