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

[refactor][java] Unify the acknowledge process for batch and non-batch message IDs #17833

Merged

Conversation

BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented Sep 25, 2022

Motivation

When I read the acknowledge related code, I found much repeat code and
the inconsistent code style. Generally we should handle the following
cases for a given message ID:

  1. The message ID doesn't contain a valid batch index, i.e. messageId instanceof BatchMessageIdImpl returns false.
  2. Otherwise, it's a BatchMessageIdImpl. The behavior is determined by
    whether the batch index ACK is enabled.

For individual ACKs, the stats and unacked message tracker must be
updated according to the type of message ID. Since MessageIdImpl is
used as the key, for a BatchMessageIdImpl, we must create a
MessageIdImpl from it as the key of these hash maps.

The existing methods modifyBatchMessageIdAndStatesInConsumer and
modifyMessageIdStatesInConsumer are confused. So I decide to make the
code simple and clear.

Modifications

  • Add a toMessageIdImpl method to BatchMessageIdImpl to create a
    MessageIdImpl object from it.
  • Add an addAcknowledgement overload to accept an extra
    BatchMessageIdImpl argument, which is used to compute the number of
    ACK commands sent and perform batch related ACKs. The MessageIdImpl
    argument is used to update the DLQ and unacked tracker.
  • Add a addIndividualAcknowledgement method for individual ACKs. This
    method accept two functions to handle ACK for batch and non-batch
    message IDs because when a list of message IDs are acknowledeged,
    doIndividualAckAsync and doIndividualBatchAck will be called
    instead of doIndividualAck and doIndividualBatchAck.

The previous modify...InConsumer methods will be merged into the
following case in addIndividualAcknowledgement:

if (batchMessageId == null || batchMessageId.ackIndividual())

Verifying this change

Rename ConsumerAckResponseTest to ConsumerAckTest. Then add some tests to verify the following states will be modified as expected.

  • The interceptor's onAcknowledge and onAcknowledgeCumulative methods.
  • The UnAckedMessageTracker instance in the consumer.
  • The numAcksSent consumer stats.

In three cases:

  • acknowledge a single message ID
  • acknowledge a list of message IDs
  • acknowledgeCumulative a single message ID

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

Matching PR in forked repository

PR in forked repository: BewareMyPower#4

@BewareMyPower BewareMyPower added area/client type/refactor Code or documentation refactors. e.g. refactor code structure or methods to improve code readability release/2.9.4 release/2.8.5 release/2.11.1 release/2.10.3 labels Sep 25, 2022
@BewareMyPower BewareMyPower self-assigned this Sep 25, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 25, 2022
@BewareMyPower BewareMyPower added type/bug The PR fixed a bug or issue reported a bug and removed type/bug The PR fixed a bug or issue reported a bug labels Sep 25, 2022
shoothzj
shoothzj previously approved these changes Sep 26, 2022
…h message IDs

### Motivation

When I read the acknowledge related code, I found much repeat code and
the inconsistent code style. Generally we should handle the following
cases for a given message ID:
1. The message ID doesn't contain a valid batch index, i.e. `messageId
   instanceof BatchMessageIdImpl` returns false.
2. Otherwise, it's a `BatchMessageIdImpl`. The behavior is determined by
   whether the batch index ACK is enabled.

For individual ACKs, the stats and unacked message tracker must be
updated according to the type of message ID. Since `MessageIdImpl` is
used as the key, for a `BatchMessageIdImpl`, we must create a
`MessageIdImpl` from it as the key of these hash maps.

The existing methods `modifyBatchMessageIdAndStatesInConsumer` and
`modifyMessageIdStatesInConsumer` are confused. So I decide to make the
code simple and clear.

### Modifications

- Add a `getMessageIdImpl` method to `BatchMessageIdImpl` to create a
  `MessageIdImpl` object from it.
- Add an `addAcknowledgement` overload to accept an extra
  `BatchMessageIdImpl` argument, which is used to compute the number of
  ACK commands sent and perform batch related ACKs. The `MessageIdImpl`
  argument is used to update the DLQ and unacked tracker.
- Add a `addIndividualAcknowledgement` method for individual ACKs. This
  method accept two functions to handle ACK for batch and non-batch
  message IDs because when a list of message IDs are acknowledeged,
  `doIndividualAckAsync` and `doIndividualBatchAck` will be called
  instead of `doIndividualAck` and `doIndividualBatchAck`.

The previous `modify...InConsumer` methods will be merged into the
following case in `addIndividualAcknowledgement`:

```java
if (batchMessageId == null || batchMessageId.ackIndividual())
```
@lhotari
Copy link
Member

lhotari commented Sep 28, 2022

/pulsarbot rerun-failure-checks

@BewareMyPower
Copy link
Contributor Author

@eolivelli @shoothzj Could you take a second look?

@BewareMyPower BewareMyPower merged commit 83309ed into apache:master Oct 6, 2022
congbobo184 pushed a commit that referenced this pull request Nov 14, 2022
@congbobo184 congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Nov 14, 2022
@BewareMyPower BewareMyPower deleted the bewaremypower/refactor-ack branch November 24, 2022 07:02
liangyepianzhou pushed a commit that referenced this pull request Dec 5, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Dec 6, 2022
…h message IDs (apache#17833)

(cherry picked from commit 83309ed)
(cherry picked from commit 331008c)
congbobo184 pushed a commit to congbobo184/pulsar that referenced this pull request Dec 7, 2022
Technoboy- pushed a commit that referenced this pull request Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/client 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.8.5 release/2.9.4 release/2.10.3 release/2.11.1 type/refactor Code or documentation refactors. e.g. refactor code structure or methods to improve code readability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants