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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Sep 1, 2022
Merged

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

merged 3 commits into from
Sep 1, 2022

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
long ackedCount = 0;
long batchSize = getBatchSize(msgId);
long batchSize = 0;
Copy link
Contributor

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

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

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

@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

@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

@Technoboy- Ping
@congbobo184 Please resolve the conflicts.

1 similar comment
@Jason918
Copy link
Contributor

@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 <[email protected]>
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 <[email protected]>
(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 <[email protected]>
(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 <[email protected]>
(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 <[email protected]>
(cherry picked from commit 176b0d6)
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