[Broker] PIP:84 Redeliver command add epoch. #10478

congbobo184 · 2021-05-04T07:29:45Z

Motivation

detail in https://github.com/apache/pulsar/wiki/PIP-84-%3A-Pulsar-client%3A-Redeliver-command-add-epoch.

Verifying this change

Add the tests for it

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: (yes)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)

# Conflicts: # pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/pendingack/impl/PendingAckHandleImpl.java

# Conflicts: # pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java

# Conflicts: # managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java # pulsar-broker/src/test/java/org/apache/pulsar/broker/service/PersistentTopicE2ETest.java

codelipenghui · 2021-09-29T13:24:54Z

pulsar-common/src/main/proto/PulsarApi.proto

@@ -604,6 +605,8 @@ message CommandCloseConsumer {
 message CommandRedeliverUnacknowledgedMessages {
    required uint64 consumer_id = 1;
    repeated MessageIdData message_ids = 2;
+    optional uint64 consumer_epoch = 3 [default = 0];
+    optional uint64 request_id = 4;


Do we need the request_id, looks like we don't have a redelivery response.

@congbobo184 do we need this one?

codelipenghui · 2021-09-30T14:27:33Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ReadOnlyCursor.java

     * @see #readEntries(int)
     */
    void asyncReadEntries(int numberOfEntriesToRead, ReadEntriesCallback callback,
-                          Object ctx, PositionImpl maxPosition);
+                          Object ctx, PositionImpl maxPosition, long epoch);


Can we move the epoch into the ctx? I think it should be a context or a carrier, if want to add more ctx in the future, we do not want to add more params, maybe we can try to create a ReadEntryContext(with recycle), the consumer ref, and the epoch should ship to the ReadEntryContext

# Conflicts: # pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java

…ver_add_epoch_pip84 # Conflicts: # pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java # pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java # pulsar-client/src/main/java/org/apache/pulsar/client/impl/MessageImpl.java

…ver_add_epoch_pip84 # Conflicts: # pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java # pulsar-broker/src/test/java/org/apache/pulsar/broker/service/PersistentDispatcherFailoverConsumerTest.java

…ver_add_epoch_pip84 # Conflicts: # pulsar-client/src/main/java/org/apache/pulsar/client/impl/MultiTopicsConsumerImpl.java

eolivelli

great work

I left some minor comments

pulsar-client/src/main/java/org/apache/pulsar/client/impl/MultiTopicsConsumerImpl.java

pulsar-common/src/main/java/org/apache/pulsar/common/protocol/Commands.java

hangc0276 · 2022-01-29T06:29:05Z

pulsar-common/src/main/proto/PulsarApi.proto

@@ -526,6 +529,7 @@ message CommandMessage {
    required MessageIdData message_id = 2;
    optional uint32 redelivery_count  = 3 [default = 0];
    repeated int64 ack_set = 4;
+    optional uint64 consumer_epoch = 5;


The default epoch is -1L, but the consumer_epoch defined in pb is uint64, it will be the max value of uint64?

To prevent use of epoch in other ways in future so we set this to uint64

pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java

hangc0276 · 2022-01-29T07:11:56Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java

+            // we need to keep both epochs the same
+            if (conf.getSubscriptionType() == SubscriptionType.Failover
+                    || conf.getSubscriptionType() == SubscriptionType.Exclusive) {
+                CONSUMER_EPOCH.incrementAndGet(this);


For MultiTopicsConsumer, it increased CONSUMER_EPOCH, and then call redeliverUnacknowledgedMessages for each consumer, will it lead epoch in consistent between consumers?

every ConsumerImpl in MultiTopicsConsumerImpl will increase the CONSUMER_EPOCH, they will change at the same time.

eolivelli

+1, as soon as other comments from other reviewers are addressed

hangc0276

Good job.

apache/pulsar#10478 introduced an API change to `MessageImpl#create`, which is included in 2.10.0.0-rc4, we need to fix the conflict.

apache/pulsar#10478 introduced an API change to `MessageImpl#create`, which is included in 2.10.0.0-rc4, we need to fix the conflict. (cherry picked from commit 857a379)

## Motivation detail in https://github.com/apache/pulsar/wiki/PIP-84-%3A-Pulsar-client%3A-Redeliver-command-add-epoch. ### Verifying this change Add the tests for it Does this pull request potentially affect one of the following parts: If yes was chosen, please highlight the changes

#10478 ### Motivation since #10478 merged, we should change the cumulative ack with transaction abort redeliver logic. We can't redeliver unCumulativeAck message by the server because the client will receive the new message and ack then will receive the old message they abort. in this case: 1. we have 5 message 2. cumulative ack 3 messages with the transaction 3. we abort this transaction 4. server redeliver message by the current consumer_epoch 5. the client will not filter the 4 or 5 messages, because in #10478 we don't change the client consumer epoch 6. client cumulative ack 4 5 with transaction and commit will lose the 1 2 3 messages and the consume message, not in order. ### Modifications don't redeliver any cumulative ack messages, it will do by user self

apache#10478 ### Motivation since apache#10478 merged, we should change the cumulative ack with transaction abort redeliver logic. We can't redeliver unCumulativeAck message by the server because the client will receive the new message and ack then will receive the old message they abort. in this case: 1. we have 5 message 2. cumulative ack 3 messages with the transaction 3. we abort this transaction 4. server redeliver message by the current consumer_epoch 5. the client will not filter the 4 or 5 messages, because in apache#10478 we don't change the client consumer epoch 6. client cumulative ack 4 5 with transaction and commit will lose the 1 2 3 messages and the consume message, not in order. ### Modifications don't redeliver any cumulative ack messages, it will do by user self

[Broker] PIP:84 Redeliver command add epoch.

ca5520e

congbobo184 requested review from codelipenghui, eolivelli, merlimat, gaoran10 and sijie May 6, 2021 12:55

codelipenghui assigned congbobo184 May 11, 2021

codelipenghui added this to the 2.8.0 milestone May 11, 2021

congbo added 5 commits May 20, 2021 18:25

Merge branch 'master' into coongbobo184_redeliver_add_epoch_pip84

696aaf9

# Conflicts: # pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/pendingack/impl/PendingAckHandleImpl.java

Fix sme code style

1f6a407

fix some code style

586a049

Merge branch 'master' into coongbobo184_redeliver_add_epoch_pip84

c309da4

# Conflicts: # pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java

Merge branch 'master' into coongbobo184_redeliver_add_epoch_pip84

352d84e

# Conflicts: # managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java # pulsar-broker/src/test/java/org/apache/pulsar/broker/service/PersistentTopicE2ETest.java

codelipenghui modified the milestones: 2.8.0, 2.9.0 Jun 1, 2021

codelipenghui reviewed Sep 29, 2021

View reviewed changes

congbo added 2 commits September 30, 2021 18:11

Merge master

b3659d4

Merge master

b0678ab

congbobo184 requested review from hangc0276 and 315157973 September 30, 2021 10:58

congbobo184 added release/2.8.2 cherry-picked/branch-2.8 Archived: 2.8 is end of life labels Sep 30, 2021

Fix code style

e08b411

codelipenghui reviewed Sep 30, 2021

View reviewed changes

eolivelli modified the milestones: 2.9.0, 2.10.0 Oct 6, 2021

congbo added 4 commits October 9, 2021 16:16

Merge branch 'master' into coongbobo184_redeliver_add_epoch_pip84

7986428

# Conflicts: # pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java

add read entry callbackWrapper

f8fb927

fix some code style

03ddf51

Change consumer set consumerEpoch = 0

278e66e

sijie mentioned this pull request Jan 25, 2022

ISSUE-13945: Consumer reconnect may lose message when cumulative ack. streamnative/pulsar-archived#3633

Open

congbo added 6 commits January 26, 2022 11:31

Fix some test

e0913ce

Merge remote-tracking branch 'apache/master' into coongbobo184_redeli…

279c770

…ver_add_epoch_pip84 # Conflicts: # pulsar-client/src/main/java/org/apache/pulsar/client/impl/MultiTopicsConsumerImpl.java

Merge master

cfe99cb

fix some test

f46fb0e

eolivelli reviewed Jan 28, 2022

View reviewed changes

hangc0276 reviewed Jan 29, 2022

View reviewed changes

Fix some comments

f4d0781

eolivelli approved these changes Jan 30, 2022

View reviewed changes

Fix consumerEpoch -1

8b6c97f

codelipenghui approved these changes Feb 14, 2022

View reviewed changes

hangc0276 approved these changes Feb 14, 2022

View reviewed changes

codelipenghui merged commit f89d649 into apache:master Feb 14, 2022

BewareMyPower mentioned this pull request Feb 15, 2022

Bump Pulsar version to 2.10.0.0-rc4 streamnative/kop#1084

Merged

BewareMyPower added a commit to streamnative/kop that referenced this pull request Feb 15, 2022

Bump Pulsar version to 2.10.0.0-rc4 (#1084)

857a379

apache/pulsar#10478 introduced an API change to `MessageImpl#create`, which is included in 2.10.0.0-rc4, we need to fix the conflict.

mattisonchao mentioned this pull request Feb 15, 2022

Bump version 2.10.0.0 rc4 streamnative/mop#457

Merged

lhotari mentioned this pull request Feb 15, 2022

lh enable codecov for unit tests lhotari/pulsar#55

Closed

congbobo184 mentioned this pull request Feb 18, 2022

[fix][txn] Transaction cumulative ack redeliver change #14371

Merged

1 task

congbobo184 deleted the coongbobo184_redeliver_add_epoch_pip84 branch March 24, 2022 04:59

mattisonchao mentioned this pull request May 24, 2022

[fix][client] Fix internal receive used wrong timeout type #15014

Merged

3 tasks

congbobo184 mentioned this pull request Jul 15, 2022

[Messaging] Cumulative ack may lose message. #10431

Closed

shibd mentioned this pull request Oct 28, 2022

[feature request] C++ and Python client consumer add epoch. apache/pulsar-client-cpp#94

Open

2 tasks

sijie mentioned this pull request Aug 19, 2022

ISSUE-17174: [feature request] C++ and Python client consumer add epoch. streamnative/pulsar-archived#4758

Open

2 tasks

lhotari mentioned this pull request Jun 11, 2024

[improve][client]PIP-359:Support custom message listener executor for specific subscription #22861

Open

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Broker] PIP:84 Redeliver command add epoch. #10478

[Broker] PIP:84 Redeliver command add epoch. #10478

congbobo184 commented May 4, 2021 •

edited

Loading

codelipenghui Sep 29, 2021

codelipenghui Dec 29, 2021

codelipenghui Sep 30, 2021

eolivelli left a comment

hangc0276 Jan 29, 2022

congbobo184 Jan 29, 2022

hangc0276 Jan 29, 2022

congbobo184 Jan 29, 2022

eolivelli left a comment

hangc0276 left a comment

[Broker] PIP:84 Redeliver command add epoch. #10478

[Broker] PIP:84 Redeliver command add epoch. #10478

Conversation

congbobo184 commented May 4, 2021 • edited Loading

Motivation

Verifying this change

codelipenghui Sep 29, 2021

Choose a reason for hiding this comment

codelipenghui Dec 29, 2021

Choose a reason for hiding this comment

codelipenghui Sep 30, 2021

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

hangc0276 Jan 29, 2022

Choose a reason for hiding this comment

congbobo184 Jan 29, 2022

Choose a reason for hiding this comment

hangc0276 Jan 29, 2022

Choose a reason for hiding this comment

congbobo184 Jan 29, 2022

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

hangc0276 left a comment

Choose a reason for hiding this comment

congbobo184 commented May 4, 2021 •

edited

Loading