You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/04/25 12:37:37 UTC

[GitHub] [pulsar] Technoboy- opened a new pull request, #15312: [fix][client] Fix negative ack not redelivery.

Technoboy- opened a new pull request, #15312:
URL: https://github.com/apache/pulsar/pull/15312

   ### Motivation
   When consumer subscribes to more than one topic, if uses negative API to redelivery msg, it will fail with the below error:
   ```
   java.lang.IllegalArgumentException: null
   	at com.google.common.base.Preconditions.checkArgument(Preconditions.java:131) ~[guava-31.0.1-jre.jar:?]
   	at org.apache.pulsar.client.impl.ConsumerImpl.redeliverUnacknowledgedMessages(ConsumerImpl.java:1901) ~[classes/:?]
   	at org.apache.pulsar.client.impl.NegativeAcksTracker.triggerRedelivery(NegativeAcksTracker.java:82) ~[classes/:?]
   ```
   
   Because if subscribes with multi-topics, the internal message will be `TopicMessageIdImpl` and add to NegativeAcksTracker without conversion to MessageIdImpl(user not config with `negativeAckRedeliveryBackoff`):
   https://github.com/apache/pulsar/blob/35c4b68f586774d0e2b7a3a2a6ab1d6a20ac1452/pulsar-client/src/main/java/org/apache/pulsar/client/impl/NegativeAcksTracker.java#L87-L112
   
   Then NegativeAcksTracker will trigger redeliver and throw IllegalArgumentException at here :
   https://github.com/apache/pulsar/blob/35c4b68f586774d0e2b7a3a2a6ab1d6a20ac1452/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java#L1896-L1902
   
   ### Documentation
   
   - [x] `no-need-doc` 
   (Please explain why)
   
   
   ### Reproduce Test
   
   ```
   @Test
       @SneakyThrows
       public void testNegativeAcks() {
           String topic = BrokerTestUtil.newUniqueName("testNegativeAcks");
           @Cleanup
           Consumer<String> consumer = pulsarClient.newConsumer(Schema.STRING)
                   .topic(topic)
                   .subscriptionName("sub1")
                   .subscriptionType(SubscriptionType.Shared)
                   .enableRetry(true)
                   .negativeAckRedeliveryDelay(1, TimeUnit.SECONDS)
                   .subscribe();
   
           @Cleanup
           Producer<String> producer = pulsarClient.newProducer(Schema.STRING)
                   .topic(topic)
                   .create();
   
           producer.sendAsync("test-ack");
           producer.flush();
           while(true) {
               Message<String> msg1 = consumer.receive();
               System.out.println("received : " + msg1);
               consumer.negativeAcknowledge(msg1);
           }
       }
   ```
     


-- 
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

[GitHub] [pulsar] mattisonchao commented on a diff in pull request #15312: [fix][client] Fix negative ack not redelivery.

Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #15312:
URL: https://github.com/apache/pulsar/pull/15312#discussion_r857645032


##########
pulsar-broker/src/test/java/org/apache/pulsar/client/impl/NegativeAcksTest.java:
##########
@@ -106,6 +106,9 @@ public void testNegativeAcks(boolean batching, boolean usePartitions, Subscripti
         log.info("Test negative acks batching={} partitions={} subType={} negAckDelayMs={}", batching, usePartitions,
                 subscriptionType, negAcksDelayMillis);
         String topic = BrokerTestUtil.newUniqueName("testNegativeAcks");
+        if (usePartitions) {
+            admin.topics().createPartitionedTopic(topic, 2);

Review Comment:
   Question: 
   I'm not sure if the topics that have been created before are cleaned up every time we run the tests? Will this affect the accuracy of the test?



-- 
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

[GitHub] [pulsar] Technoboy- commented on a diff in pull request #15312: [fix][client] Fix negative ack not redelivery.

Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15312:
URL: https://github.com/apache/pulsar/pull/15312#discussion_r857620698


##########
pulsar-broker/src/test/java/org/apache/pulsar/client/impl/NegativeAcksTest.java:
##########
@@ -106,6 +106,7 @@ public void testNegativeAcks(boolean batching, boolean usePartitions, Subscripti
         log.info("Test negative acks batching={} partitions={} subType={} negAckDelayMs={}", batching, usePartitions,
                 subscriptionType, negAcksDelayMillis);
         String topic = BrokerTestUtil.newUniqueName("testNegativeAcks");
+        admin.topics().createPartitionedTopic(topic, 2);

Review Comment:
   yes, right. Fixed.



-- 
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

[GitHub] [pulsar] Technoboy- commented on pull request #15312: [fix][client] Fix negative ack not redelivery.

Posted by GitBox <gi...@apache.org>.
Technoboy- commented on PR #15312:
URL: https://github.com/apache/pulsar/pull/15312#issuecomment-1108525628

   > @Technoboy- can you check #6869 , does this PR fix that issue?
   
   It's not related to that issue. This bug was introduced by #12566.  


-- 
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

[GitHub] [pulsar] Technoboy- merged pull request #15312: [fix][client] Fix negative ack not redelivery.

Posted by GitBox <gi...@apache.org>.
Technoboy- merged PR #15312:
URL: https://github.com/apache/pulsar/pull/15312


-- 
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

[GitHub] [pulsar] Technoboy- commented on pull request #15312: [fix][client] Fix negative ack not redelivery.

Posted by GitBox <gi...@apache.org>.
Technoboy- commented on PR #15312:
URL: https://github.com/apache/pulsar/pull/15312#issuecomment-1110924455

   > > It's not related to that issue. This bug was introduced by #12566.
   > 
   > thanks. btw. would you be interested in fixing #6869 ? That's a long time issue with negative acknowledgements and batch messages.
   
   Hi @lhotari , I think #6869 has been fixed by #9440. 


-- 
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

[GitHub] [pulsar] lhotari commented on pull request #15312: [fix][client] Fix negative ack not redelivery.

Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #15312:
URL: https://github.com/apache/pulsar/pull/15312#issuecomment-1108529247

   > It's not related to that issue. This bug was introduced by #12566.
   
   thanks. btw. would you be interested in fixing #6869 ? That's a long time issue with negative acknowledgements and batch messages. 
   


-- 
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

[GitHub] [pulsar] lhotari commented on pull request #15312: [fix][client] Fix negative ack not redelivery.

Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #15312:
URL: https://github.com/apache/pulsar/pull/15312#issuecomment-1108523930

   @Technoboy- can you check #6869 , does this PR fix that issue?


-- 
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

[GitHub] [pulsar] Technoboy- commented on a diff in pull request #15312: [fix][client] Fix negative ack not redelivery.

Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15312:
URL: https://github.com/apache/pulsar/pull/15312#discussion_r857649071


##########
pulsar-broker/src/test/java/org/apache/pulsar/client/impl/NegativeAcksTest.java:
##########
@@ -106,6 +106,9 @@ public void testNegativeAcks(boolean batching, boolean usePartitions, Subscripti
         log.info("Test negative acks batching={} partitions={} subType={} negAckDelayMs={}", batching, usePartitions,
                 subscriptionType, negAcksDelayMillis);
         String topic = BrokerTestUtil.newUniqueName("testNegativeAcks");
+        if (usePartitions) {
+            admin.topics().createPartitionedTopic(topic, 2);

Review Comment:
   `newUniqueName ` will use UUID to create topic.



-- 
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

[GitHub] [pulsar] Technoboy- commented on pull request #15312: [fix][client] Fix negative ack not redelivery.

Posted by GitBox <gi...@apache.org>.
Technoboy- commented on PR #15312:
URL: https://github.com/apache/pulsar/pull/15312#issuecomment-1108548095

   > > It's not related to that issue. This bug was introduced by #12566.
   > 
   > thanks. btw. would you be interested in fixing #6869 ? That's a long time issue with negative acknowledgements and batch messages.
   
   I will work on it.


-- 
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

[GitHub] [pulsar] codelipenghui commented on a diff in pull request #15312: [fix][client] Fix negative ack not redelivery.

Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #15312:
URL: https://github.com/apache/pulsar/pull/15312#discussion_r857609830


##########
pulsar-broker/src/test/java/org/apache/pulsar/client/impl/NegativeAcksTest.java:
##########
@@ -106,6 +106,7 @@ public void testNegativeAcks(boolean batching, boolean usePartitions, Subscripti
         log.info("Test negative acks batching={} partitions={} subType={} negAckDelayMs={}", batching, usePartitions,
                 subscriptionType, negAcksDelayMillis);
         String topic = BrokerTestUtil.newUniqueName("testNegativeAcks");
+        admin.topics().createPartitionedTopic(topic, 2);

Review Comment:
   Should apply `usePartitions` to run test on partitioned topic and non-partitioned topic



-- 
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

[GitHub] [pulsar] mattisonchao commented on a diff in pull request #15312: [fix][client] Fix negative ack not redelivery.

Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #15312:
URL: https://github.com/apache/pulsar/pull/15312#discussion_r857650958


##########
pulsar-broker/src/test/java/org/apache/pulsar/client/impl/NegativeAcksTest.java:
##########
@@ -106,6 +106,9 @@ public void testNegativeAcks(boolean batching, boolean usePartitions, Subscripti
         log.info("Test negative acks batching={} partitions={} subType={} negAckDelayMs={}", batching, usePartitions,
                 subscriptionType, negAcksDelayMillis);
         String topic = BrokerTestUtil.newUniqueName("testNegativeAcks");
+        if (usePartitions) {
+            admin.topics().createPartitionedTopic(topic, 2);

Review Comment:
   Oh, thanks.



-- 
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