coderzc opened a new pull request, #18531:
URL: https://github.com/apache/pulsar/pull/18531

   ### Motivation
   When enable batch index acknowledgment, we are unable to send a cumulative 
acknowledgment for the last messageId of a batch, the root cause is the last 
messageId of a batch doesn't carry with batch index but due to 
`org.apache.pulsar.client.impl.LastCumulativeAck#update` the wrong comparison 
make the last messageId can't be sent.
   
   
https://github.com/apache/pulsar/blob/545f33fa35309510585e28283ba485a60deddf15/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java#L228-L229
   
   
https://github.com/apache/pulsar/blob/545f33fa35309510585e28283ba485a60deddf15/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java#L624-L632
   
   ```java
       public synchronized void update(final MessageIdImpl messageId, final 
BitSetRecyclable bitSetRecyclable) {
           if (messageId.compareTo(this.messageId) > 0) {
           // messageId(4,0,0) < this.messageId(4,0,0,8) so the following logic 
cannot be executed
               if (this.bitSetRecyclable != null && this.bitSetRecyclable != 
bitSetRecyclable) {
                   this.bitSetRecyclable.recycle();
               }
               set(messageId, bitSetRecyclable);
               flushRequired = true;
           }
       }
   ```
   
   
   ### Modifications
   
   Fix the wrong comparison in 
`org.apache.pulsar.client.impl.LastCumulativeAck#update` to `(x,y,z) >= 
(x,y,z,w)`
   
   ### Verifying this change
   
   - [x] Make sure that the change passes the CI checks.
   
   *(Please pick either of the following options)*
   
   This change is a trivial rework / code cleanup without any test coverage.
   
   *testAcknowledgeCumulative*
   
   ### Documentation
   
   <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
   
   - [ ] `doc` <!-- Your PR contains doc changes. Please attach the local 
preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR 
description, or else your PR might not get merged. -->
   - [ ] `doc-required` <!-- Your PR changes impact docs and you will update 
later -->
   - [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
   - [ ] `doc-complete` <!-- Docs have been already added -->
   
   ### Matching PR in forked repository
   
   PR in forked repository: 
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to