[GitHub] [pulsar] poorbarcode commented on a diff in pull request #20591: [fix] [client] Messages lost when consumer reconnect

["poorbarcode (via GitHub)" <gi...@apache.org>]

poorbarcode commented on code in PR #20591:
URL: https://github.com/apache/pulsar/pull/20591#discussion_r1232010411


##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java:
##########
@@ -136,6 +136,7 @@
 
     protected volatile MessageId lastDequeuedMessageId = MessageId.earliest;
     private volatile MessageId lastMessageIdInBroker = MessageId.earliest;
+    protected volatile CompletableFuture<Void> inProgressSubscribeFuture;

Review Comment:
   I agree with you. The best way is to protect grabCnx, such as this:
   - add a new [HandlerState](https://github.com/apache/pulsar/blob/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/HandlerState.java) `Registering(means sending "Subscribe" or "AddProducer" to broker)` between `Connecting` and `Reday`.
   - `connectionOpened` is called only when comparing and swap(no reentrant) `Connecting -> Registering` successful
   
   But if we did the change above, It would make a large impact. I suggest doing a fast fix first(and cherry-pick it into the previous branches), then doing the perfect changes above just in the newest release. 



-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org