Skip to content

[fix][broker] fix incorrect unack stats when dup ack a message#19348

Closed
labuladong wants to merge 2 commits intoapache:masterfrom
labuladong:fix/issue/19268
Closed

[fix][broker] fix incorrect unack stats when dup ack a message#19348
labuladong wants to merge 2 commits intoapache:masterfrom
labuladong:fix/issue/19268

Conversation

@labuladong
Copy link
Contributor

@labuladong labuladong commented Jan 29, 2023

Fixes #19268

Modifications

For shared-type consumers, if can't find the message position in pendingAck set, skip the following process.

Verifying this change

  • Make sure that the change passes the CI checks.

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

  • 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

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: labuladong#20

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 29, 2023
@codelipenghui codelipenghui added this to the 3.0.0 milestone Jan 30, 2023
Comment on lines +476 to +480
if (Subscription.isIndividualAckMode(ackOwnerConsumer.subType())
&& !ackOwnerConsumer.getPendingAcks().containsKey(msgId.getLedgerId(), msgId.getEntryId())) {
// this message has been acked, skip to keep stats correct
continue;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ackOwnerConsumer can be a different consumer.

private Consumer getAckOwnerConsumer(long ledgerId, long entryId) {
    Consumer ackOwnerConsumer = this;
    if (Subscription.isIndividualAckMode(subType)) {
        if (!getPendingAcks().containsKey(ledgerId, entryId)) {
            for (Consumer consumer : subscription.getConsumers()) {
                if (consumer != this && consumer.getPendingAcks().containsKey(ledgerId, entryId)) {
                    ackOwnerConsumer = consumer;
                    break;
                }
            }
        }
    }
    return ackOwnerConsumer;
}

We will still have the issue that the unacked messages count is decreased for this consumer, but actually the owner consumer is another consumer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand, if the actual consumer is another consumer, addAndGetUnAckedMsgs can update the unack count of that consumer:

private int addAndGetUnAckedMsgs(Consumer consumer, int ackedMessages) {
int unackedMsgs = 0;
if (Subscription.isIndividualAckMode(subType)) {
subscription.addUnAckedMessages(ackedMessages);
unackedMsgs = UNACKED_MESSAGES_UPDATER.addAndGet(consumer, ackedMessages);
}
if (unackedMsgs < 0 && System.currentTimeMillis() - negtiveUnackedMsgsTimestamp >= 10_000) {
negtiveUnackedMsgsTimestamp = System.currentTimeMillis();
log.warn("unackedMsgs is : {}, ackedMessages : {}, consumer : {}", unackedMsgs, ackedMessages, consumer);
}
return unackedMsgs;
}

@github-actions
Copy link

github-actions bot commented Mar 9, 2023

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Mar 9, 2023
@poorbarcode
Copy link
Contributor

Since we will start the RC version of 3.0.0 on 2023-04-11, I will change the label/milestone of PR who have not been merged.

  • The PR of type feature is deferred to 3.1.0
  • The PR of type fix is deferred to 3.0.1

So drag this PR to 3.0.1

@poorbarcode poorbarcode modified the milestones: 3.0.0, 3.1.0 Apr 10, 2023
@github-actions github-actions bot removed the Stale label Apr 11, 2023
@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@nodece
Copy link
Member

nodece commented Jan 11, 2024

#19268 has been fixed by #20990, so close this PR.

@nodece nodece closed this Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs release/3.0.3 Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Acknowledge the same message-id causes inconsistency unackedMessages number.

6 participants