Skip to content

[fix][txn] fix ack with txn compute ackedCount error#17016

Merged
Jason918 merged 3 commits intoapache:masterfrom
congbobo184:congbobo184_fix_transaction_ack_reduce_unack_message_count
Sep 1, 2022
Merged

[fix][txn] fix ack with txn compute ackedCount error#17016
Jason918 merged 3 commits intoapache:masterfrom
congbobo184:congbobo184_fix_transaction_ack_reduce_unack_message_count

Conversation

@congbobo184
Copy link
Contributor

@congbobo184 congbobo184 commented Aug 9, 2022 •
edited by github-actions bot
Loading

issue:
reproduce:

  1. redeliver one consumer received message of a batch message, will remove this message from this consumer pending acks
  2. if this message is removed and doesn't redeliver to another consumer or this consumer, we ack another message in the same batch
  3. to compute the consumer unackedCount will get an incorrect batch size, because the message don't in any consumer pending acks

Motivation

get correct batch size to compute the consumer unackedCount

Modification

use the correct batch size from the messageIdDate

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 this to the 2.11.0 milestone Aug 9, 2022
@congbobo184 congbobo184 self-assigned this Aug 9, 2022
liangyepianzhou
liangyepianzhou reviewed Aug 9, 2022
View reviewed changes
Copy link
Contributor

@liangyepianzhou liangyepianzhou left a comment

Choose a reason for hiding this comment

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

Maybe we can also change get batchSize in the method individualAckNormal.

@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
gaoran10
gaoran10 reviewed Aug 11, 2022
View reviewed changes
Copy link
Contributor

@gaoran10 gaoran10 left a comment

Choose a reason for hiding this comment

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

Great work! Left some comments.

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java
// acked count at least one
long ackedCount = 0;
long batchSize = getBatchSize(msgId);
long batchSize = 0;
Copy link
Contributor

@gaoran10 gaoran10 Aug 11, 2022

Choose a reason for hiding this comment

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

The default value may be 1.

Copy link
Contributor Author

@congbobo184 congbobo184 Aug 12, 2022

Choose a reason for hiding this comment

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

if not batch message, the batchSize can not be able to 1

Copy link
Contributor

@gaoran10 gaoran10 Aug 15, 2022

Choose a reason for hiding this comment

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

Ok, If not batch message, the batch size should be 0, right?

@congbobo184 congbobo184 changed the title [fix][broker] fix ack with txn compute ackedCount error [fix][txn] fix ack with txn compute ackedCount error Aug 15, 2022
@github-actions github-actions bot removed the doc-not-needed Your PR changes do not impact docs label Aug 15, 2022
@github-actions
Copy link

github-actions bot commented Aug 15, 2022

@congbobo184 Please provide a correct documentation label for your PR.
Instructions see Pulsar Documentation Label Guide.

gaoran10
gaoran10 approved these changes Aug 15, 2022
View reviewed changes
Copy link
Contributor

@gaoran10 gaoran10 left a comment

Choose a reason for hiding this comment

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

LGTM

@Technoboy- Technoboy- added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Aug 15, 2022
Technoboy-
Technoboy- reviewed Aug 15, 2022
View reviewed changes
Copy link
Contributor

@Technoboy- Technoboy- left a comment

Choose a reason for hiding this comment

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

When sendMessages, we put the batch size into pendingAcks, so in general, we may get the batch size from it when process ack...

@congbobo184
Copy link
Contributor Author

congbobo184 commented Aug 16, 2022

@Technoboy- transaction ack command will carry the batchSize, if the batchSize is not in pendingAcks(the message redeliver hasn't been sent to any consumer), we will not get the correct batch size. So I think in Transaction ack, we uniformly use command carried batch size is ok

@Jason918
Copy link
Contributor

Jason918 commented Aug 27, 2022

@Technoboy- Ping
@congbobo184 Please resolve the conflicts.

1 similar comment
@Jason918
Copy link
Contributor

Jason918 commented Aug 27, 2022

@Technoboy- Ping
@congbobo184 Please resolve the conflicts.

Merge remote-tracking branch 'apache/master' into congbobo184_fix_tra…
c9f43c8 
…nsaction_ack_reduce_unack_message_count

# Conflicts:
#	pulsar-broker/src/test/java/org/apache/pulsar/client/impl/TransactionEndToEndTest.java
@Jason918 Jason918 merged commit 176b0d6 into apache:master Sep 1, 2022
Technoboy- pushed a commit that referenced this pull request Sep 1, 2022
@congbobo184 @Technoboy-
[fix][txn] fix ack with txn compute ackedCount error (#17016)
d9cd5f4 
Co-authored-by: congbobo184 <congbobo184@github.com>
Jason918 pushed a commit that referenced this pull request Sep 4, 2022
@congbobo184 @Jason918
[fix][txn] fix ack with txn compute ackedCount error (#17016)
a0eb84e 
Co-authored-by: congbobo184 <congbobo184@github.com>
(cherry picked from commit 176b0d6)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Sep 16, 2022
@congbobo184 @nicoloboschi
[fix][txn] fix ack with txn compute ackedCount error (apache#17016)
b11181f 
Co-authored-by: congbobo184 <congbobo184@github.com>
(cherry picked from commit 176b0d6)
(cherry picked from commit a0eb84e)
congbobo184 added a commit that referenced this pull request Nov 8, 2022
@congbobo184
[fix][txn] fix ack with txn compute ackedCount error (#17016)
cff0e83 
Co-authored-by: congbobo184 <congbobo184@github.com>
(cherry picked from commit 176b0d6)
@congbobo184 congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Nov 8, 2022
congbobo184 added a commit that referenced this pull request Nov 30, 2022
@congbobo184
[fix][txn] fix ack with txn compute ackedCount error (#17016)
91dac3e 
Co-authored-by: congbobo184 <congbobo184@github.com>
(cherry picked from commit 176b0d6)
This was referenced Dec 22, 2025
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liangyepianzhou liangyepianzhou liangyepianzhou left review comments

@Technoboy- Technoboy- Technoboy- approved these changes

@gaoran10 gaoran10 gaoran10 approved these changes

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

@mattisonchao mattisonchao Awaiting requested review from mattisonchao

Assignees

@congbobo184 congbobo184

Labels

area/broker area/transaction cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 cherry-picked/branch-2.11 doc-not-needed Your PR changes do not impact docs release/2.9.4 release/2.10.2

Projects

None yet

Milestone

2.11.0

Development

Successfully merging this pull request may close these issues.

5 participants

@congbobo184 @Jason918 @Technoboy- @gaoran10 @liangyepianzhou