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

[improve][client] improve logic when ACK grouping tracker checks duplicated message id #15465

Merged
merged 1 commit into from
May 7, 2022
Merged

[improve][client] improve logic when ACK grouping tracker checks duplicated message id #15465

merged 1 commit into from
May 7, 2022

Conversation

mattisonchao
Copy link
Member

Motivation

#10586 introduces NPE check to avoid problems caused by recycling race conditions, but another conditional judgment in isDuplicate logic is not strongly dependent on the messageIdOfLastAck, we can still use pendingIndividualAcks.contains(messageId) to judge again.

Modifications

  • Use pendingIndividualAcks.contains(messageId) to judgement when messageIdOfLastAck is null.

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

  • no-need-doc

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 6, 2022
@mattisonchao
Copy link
Member Author

@BewareMyPower
Thinking about it carefully, it seems that there is no problem with the current implementation, but it feels a little strange. Please let me know what you think, thanks.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

I am sorrry but I don't understanding the benefit of this patch.
We are not saving CPU cycles or fixing a bug.

Usually it is better to not change the code if there isn't a clear benefit.
'Don't fix things that are not broken'

@gaozhangmin
Copy link
Contributor

I am sorrry but I don't understanding the benefit of this patch. We are not saving CPU cycles or fixing a bug.

Usually it is better to not change the code if there isn't a clear benefit. 'Don't fix things that are not broken'

+1

@mattisonchao
Copy link
Member Author

Hi, @eolivelli
I just have a small concern about it. If it's don't need to care about it, I will close this PR.

PR #10586 looks to change the original design logic when messageId is null. In the original logic, messageIdOfLastAck is initialized as -1:-1:-1, so we use this logic to ensure that we can use both messageIdOfLastAck and pendingIndividualAcks to deduplicate messages. (Of course, the original author did not consider the race condition).

public boolean isDuplicate(MessageId messageId) {
        if (messageId.compareTo(lastCumulativeAck) <= 0) {
            // Already included in a cumulative ack
            return true;
        } else {
            return pendingIndividualAcks.contains(messageId);
        }
    }

But, in the current implementation, If messageIdOfLastAck is null, we don't use pendingIndividualAcks to filter messages.(pendingIndividualAcks not rely on messageIdOfLastAck)

    public boolean isDuplicate(@NonNull MessageId messageId) {
        final MessageId messageIdOfLastAck = lastCumulativeAck.messageId;
        if (messageIdOfLastAck == null) {
            return false;
        }
        if (messageId.compareTo(messageIdOfLastAck) <= 0) {
            // Already included in a cumulative ack
            return true;
        } else {
            return pendingIndividualAcks.contains(messageId);
        }
    }

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Thanks for your clarification.

It would have been better do add a test case, but I understand it is something very hard to reproduce

@mattisonchao mattisonchao merged commit f6faeec into apache:master May 7, 2022
@mattisonchao mattisonchao deleted the fix_wrong_client_logic branch May 7, 2022 07:35
codelipenghui pushed a commit that referenced this pull request May 20, 2022
@codelipenghui codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label May 20, 2022
codelipenghui pushed a commit that referenced this pull request May 20, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request May 23, 2022
…icated message id (apache#15465)

(cherry picked from commit f6faeec)
(cherry picked from commit d91c6d1)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request May 23, 2022
…icated message id (apache#15465)

(cherry picked from commit f6faeec)
(cherry picked from commit 8b4d81e)
mattisonchao added a commit that referenced this pull request May 25, 2022
@mattisonchao mattisonchao added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label May 25, 2022
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.

6 participants