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 unack message count for transaction Ack while disabled batch index ack #14071

Merged

Conversation

codelipenghui
Copy link
Contributor

@codelipenghui codelipenghui commented Jan 30, 2022

Motivation

Fix unack message count for transaction Ack while disabled batch index ack.

Transaction Ack is different with normal message ack for a batch message.

For normal message, we are using a bitset to carry the batch index state, for example

1. Ack with `00111111` means acks batch index 0 and 1
2. For ack batch index 2 and 3, the client will send `00001111` to broker
3. After all the batch been acked, send `00000000` to broker

The following is for transaction ack:

1. `00111111` means acks batch index 0 and 1
2. `11001111` means acks batch index 2 and 3

Verification

Enabled transaction e2e test for batch index ack disabled

Documentation

Check the box below or label this PR directly (if you have committer privilege).

Need to update docs?

  • doc-required

    (If you need help on updating docs, create a doc issue)

  • no-need-doc

    (Please explain why)

  • doc

    (If this PR contains doc changes)

…x ack.

### Motivation

Fix unack message count for transaction Ack while disabled batch index ack.

Transaction Ack is different with normal message ack for a batch message.

For normal message, we are using a bitset to carry the batch index state, for example

```
1. Ack with `00111111` means acks batch index 0 and 1
2. For ack batch index 2 and 3, the client will send `00001111` to broker
3. After all the batch been acked, send `00000000` to broker
```

The following is for transaction ack:

```
1. `00111111` means acks batch index 0 and 1
1. `11001111` means acks batch index 2 and 3
```

### Verification

Enabled transaction e2e test for batch index ack disabled
@codelipenghui codelipenghui added this to the 2.10.0 milestone Jan 30, 2022
@codelipenghui codelipenghui self-assigned this Jan 30, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 30, 2022
@codelipenghui codelipenghui added area/transaction release/2.9.3 type/bug The PR fixed a bug or issue reported a bug and removed doc-not-needed Your PR changes do not impact docs labels Jan 30, 2022
@github-actions
Copy link

@codelipenghui:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@Anonymitaet Anonymitaet added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Feb 7, 2022
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

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

@codelipenghui codelipenghui merged commit a640146 into apache:master Feb 10, 2022
@codelipenghui codelipenghui deleted the penghui/fix-unack-count-transaction branch February 10, 2022 02:56
codelipenghui added a commit that referenced this pull request Feb 10, 2022
…x ack (#14071)

* Fix unack message count for transaction Ack while disabled batch index ack.

### Motivation

Fix unack message count for transaction Ack while disabled batch index ack.

Transaction Ack is different with normal message ack for a batch message.

For normal message, we are using a bitset to carry the batch index state, for example

```
1. Ack with `00111111` means acks batch index 0 and 1
2. For ack batch index 2 and 3, the client will send `00001111` to broker
3. After all the batch been acked, send `00000000` to broker
```

The following is for transaction ack:

```
1. `00111111` means acks batch index 0 and 1
1. `11001111` means acks batch index 2 and 3
```

### Verification

Enabled transaction e2e test for batch index ack disabled

(cherry picked from commit a640146)
@michaeljmarshall
Copy link
Member

@codelipenghui - is this needed in 2.8? I noticed that the BatchMessageWithBatchIndexLevelTest is modified here and that created some conflicts for me when cherry picking #14288. I'm guessing 2.8 is fine, but just want to double check.

Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
…x ack (apache#14071)

* Fix unack message count for transaction Ack while disabled batch index ack.

### Motivation

Fix unack message count for transaction Ack while disabled batch index ack.

Transaction Ack is different with normal message ack for a batch message.

For normal message, we are using a bitset to carry the batch index state, for example

```
1. Ack with `00111111` means acks batch index 0 and 1
2. For ack batch index 2 and 3, the client will send `00001111` to broker
3. After all the batch been acked, send `00000000` to broker
```

The following is for transaction ack:

```
1. `00111111` means acks batch index 0 and 1
1. `11001111` means acks batch index 2 and 3
```

### Verification

Enabled transaction e2e test for batch index ack disabled
@codelipenghui codelipenghui restored the penghui/fix-unack-count-transaction branch May 17, 2022 01:21
@codelipenghui codelipenghui deleted the penghui/fix-unack-count-transaction branch May 17, 2022 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/transaction cherry-picked/branch-2.9 Archived: 2.9 is end of life doc-not-needed Your PR changes do not impact docs release/2.9.2 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants