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/03/16 03:35:44 UTC

[GitHub] [pulsar] wolfstudy opened a new pull request #14706: Process maxRedeliverCount is 0 of DeadLeddterPolicy

wolfstudy opened a new pull request #14706:
URL: https://github.com/apache/pulsar/pull/14706


   Signed-off-by: xiaolongran <xi...@tencent.com>
   
   
   Fixes #14704 
   
   
   
   ### Motivation
   
   When the user uses the function of DeadLetterPolicy, it is better than misoperation. MaxRedeliverCount may be set to 0. When it is set to 0, according to the current processing logic of the Java Client, the message will be pushed to the DeadLetter Topic every time.
   
   
   ### Modifications
   
   - When MaxRedeliverCount <= 0 in DeadLetterPolicy, we reset MaxRedeliverCount to default value
   
   


-- 
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] wolfstudy edited a comment on pull request #14706: Process maxRedeliverCount is 0 of DeadLeddterPolicy

Posted by GitBox <gi...@apache.org>.
wolfstudy edited a comment on pull request #14706:
URL: https://github.com/apache/pulsar/pull/14706#issuecomment-1068793396






-- 
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] wolfstudy merged pull request #14706: Process maxRedeliverCount is 0 of DeadLeddterPolicy

Posted by GitBox <gi...@apache.org>.
wolfstudy merged pull request #14706:
URL: https://github.com/apache/pulsar/pull/14706


   


-- 
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] github-actions[bot] commented on pull request #14706: Process maxRedeliverCount is 0 of DeadLeddterPolicy

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14706:
URL: https://github.com/apache/pulsar/pull/14706#issuecomment-1068698973


   @wolfstudy:Thanks for providing doc info!


-- 
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] github-actions[bot] commented on pull request #14706: Process maxRedeliverCount is 0 of DeadLeddterPolicy

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14706:
URL: https://github.com/apache/pulsar/pull/14706#issuecomment-1068697193


   @wolfstudy:Thanks for your contribution. For this PR, do we need to update docs?
   (The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? 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

[GitHub] [pulsar] wolfstudy commented on a change in pull request #14706: Process maxRedeliverCount is 0 of DeadLeddterPolicy

Posted by GitBox <gi...@apache.org>.
wolfstudy commented on a change in pull request #14706:
URL: https://github.com/apache/pulsar/pull/14706#discussion_r828815355



##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBuilderImpl.java
##########
@@ -437,6 +437,12 @@ public ConsumerBuilderImpl(PulsarClientImpl client, Schema<T> schema) {
             if (conf.getAckTimeoutMillis() == 0) {
                 conf.setAckTimeoutMillis(DEFAULT_ACK_TIMEOUT_MILLIS_FOR_DEAD_LETTER);
             }
+
+            // when MaxRedeliverCount <= 0 in DeadLetterPolicy, we reset MaxRedeliverCount
+            // to default value, Default: 16.
+            if (deadLetterPolicy.getMaxRedeliverCount() <= 0) {
+                deadLetterPolicy.setMaxRedeliverCount(RetryMessageUtil.MAX_RECONSUMETIMES);

Review comment:
       Thanks @Jason918 @codelipenghui review, fix done please check again




-- 
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] wolfstudy edited a comment on pull request #14706: Process maxRedeliverCount is 0 of DeadLeddterPolicy

Posted by GitBox <gi...@apache.org>.
wolfstudy edited a comment on pull request #14706:
URL: https://github.com/apache/pulsar/pull/14706#issuecomment-1068793396


   > Is it better to handle illegal user value in `org.apache.pulsar.client.impl.ConsumerBuilderImpl#deadLetterPolicy`? We already did similar stuff there.
   
   Good ideas, fix this please check again @Jason918 


-- 
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 change in pull request #14706: Process maxRedeliverCount is 0 of DeadLeddterPolicy

Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #14706:
URL: https://github.com/apache/pulsar/pull/14706#discussion_r827876211



##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBuilderImpl.java
##########
@@ -437,6 +437,12 @@ public ConsumerBuilderImpl(PulsarClientImpl client, Schema<T> schema) {
             if (conf.getAckTimeoutMillis() == 0) {
                 conf.setAckTimeoutMillis(DEFAULT_ACK_TIMEOUT_MILLIS_FOR_DEAD_LETTER);
             }
+
+            // when MaxRedeliverCount <= 0 in DeadLetterPolicy, we reset MaxRedeliverCount
+            // to default value, Default: 16.
+            if (deadLetterPolicy.getMaxRedeliverCount() <= 0) {
+                deadLetterPolicy.setMaxRedeliverCount(RetryMessageUtil.MAX_RECONSUMETIMES);

Review comment:
       It's better to add a warn log here to show users which value is applied.
   
   And need an unit test to make sure we will not break here.




-- 
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] wolfstudy merged pull request #14706: Process maxRedeliverCount is 0 of DeadLeddterPolicy

Posted by GitBox <gi...@apache.org>.
wolfstudy merged pull request #14706:
URL: https://github.com/apache/pulsar/pull/14706


   


-- 
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] Jason918 commented on a change in pull request #14706: Process maxRedeliverCount is 0 of DeadLeddterPolicy

Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #14706:
URL: https://github.com/apache/pulsar/pull/14706#discussion_r828802980



##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBuilderImpl.java
##########
@@ -437,6 +437,12 @@ public ConsumerBuilderImpl(PulsarClientImpl client, Schema<T> schema) {
             if (conf.getAckTimeoutMillis() == 0) {
                 conf.setAckTimeoutMillis(DEFAULT_ACK_TIMEOUT_MILLIS_FOR_DEAD_LETTER);
             }
+
+            // when MaxRedeliverCount <= 0 in DeadLetterPolicy, we reset MaxRedeliverCount
+            // to default value, Default: 16.
+            if (deadLetterPolicy.getMaxRedeliverCount() <= 0) {
+                deadLetterPolicy.setMaxRedeliverCount(RetryMessageUtil.MAX_RECONSUMETIMES);

Review comment:
       Maybe a `checkArgument` is better here to keep consistent with other parameter validation. @codelipenghui WDYT




-- 
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] wolfstudy commented on pull request #14706: Process maxRedeliverCount is 0 of DeadLeddterPolicy

Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #14706:
URL: https://github.com/apache/pulsar/pull/14706#issuecomment-1068793396


   > 
   
   Good ideas, fix this please check again


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