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 2023/01/04 22:00:47 UTC
[GitHub] [pulsar] klevy-toast opened a new issue, #19134: Remove default 30s ackTimeout when using deadLetterPolicy
klevy-toast opened a new issue, #19134:
URL: https://github.com/apache/pulsar/issues/19134
### Search before asking
- [X] I searched in the [issues](https://github.com/apache/pulsar/issues) and found nothing similar.
### Motivation
[Context](https://github.com/apache/pulsar/pull/3014#issuecomment-493759971) and [additional context](https://github.com/apache/pulsar/issues/8484)
The deadLetterPolicy was implemented before negativeAcknowledgement was an option, which meant that the client had to rely on ackTimeout to result in redelivery / DLQ. However, now that negativeAck is available, we should not default to using the 30s ackTimeout, since it can cause cascading redeliveries when an application takes longer than 30s to process a message. Additionally, the behavior is inconsistent by ordering in the ConsumerBuilder:
```
consumerBuilder
.ackTimeout(0, TimeUnit.MILLISECONDS)
.deadLetterPolicy(DeadLetterPolicy.builder().build())
.subscribe()
```
vs
```
consumerBuilder
.deadLetterPolicy(DeadLetterPolicy.builder().build())
.ackTimeout(0, TimeUnit.MILLISECONDS)
.subscribe()
```
Option 1 results in ackTimeout of 30s, despite disabling it explicitly, while option 2 _does_ disable ackTimeout.
### Solution
I would like to remove the default ackTimeout set [here](https://github.com/apache/pulsar/blob/aaf3a9a9a7464a5553d22ac3d8aff4352734941d/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBuilderImpl.java#L444). This was discussed already [here](https://github.com/apache/pulsar/pull/3014#issuecomment-493759971)
### Alternatives
If changing the default is not a viable solution, then we could add a flag in the ConsumerBuilder that denotes when ackTimeout has been set by the user and prevent the default from taking effect in this case, which was discussed already [here](https://github.com/apache/pulsar/issues/8484#issuecomment-726057120)
### Anything else?
_No response_
### Are you willing to submit a PR?
- [X] I'm willing to submit a PR!
--
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.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- closed issue #19134: Remove default 30s ackTimeout when using deadLetterPolicy
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- closed issue #19134: Remove default 30s ackTimeout when using deadLetterPolicy
URL: https://github.com/apache/pulsar/issues/19134
--
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] michaeljmarshall commented on issue #19134: Remove default 30s ackTimeout when using deadLetterPolicy
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on issue #19134:
URL: https://github.com/apache/pulsar/issues/19134#issuecomment-1430726479
@klevy-toast - that explanation makes sense and that behavior would definitely be problematic. Thanks for submitting the PR to fix 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] michaeljmarshall commented on issue #19134: Remove default 30s ackTimeout when using deadLetterPolicy
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on issue #19134:
URL: https://github.com/apache/pulsar/issues/19134#issuecomment-1416503871
Great write up @klevy-toast! The historical context is very interesting, and explains the current behavior. I think it is a good idea to remove these lines:
https://github.com/apache/pulsar/blob/aaf3a9a9a7464a5553d22ac3d8aff4352734941d/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBuilderImpl.java#L443-L445
To build on top of what you wrote, we should follow the principal of least surprise with our API, and updating `ackTimeoutMillis` from its default value of 0 to 30 when setting the `DeadLetterPolicy` is definitely surprising.
Further, I would not expect the order of unrelated builder method calls to affect each other.
> we should not default to using the 30s ackTimeout, since it can cause cascading redeliveries when an application takes longer than 30s to process a message.
If you are observing this behavior, that sounds like a bug. It looks like there might be a race in the logic, but the client does attempt to handle this case here:
https://github.com/apache/pulsar/blob/39dd1cda2d01a6d472b7a39115a774958a837be1/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java#L746-L752
Note: as @dave2wave mentioned in slack, a good place to create visibility on this change is on the dev@ mailing list. In my opinion, you should open a PR with your proposed change and include that PR and this issue in your email to the mailing list.
--
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] klevy-toast commented on issue #19134: Remove default 30s ackTimeout when using deadLetterPolicy
Posted by "klevy-toast (via GitHub)" <gi...@apache.org>.
klevy-toast commented on issue #19134:
URL: https://github.com/apache/pulsar/issues/19134#issuecomment-1416567954
> If you are observing this behavior, that sounds like a bug. It looks like there might be a race in the logic, but the client does attempt to handle this case here:
>
> https://github.com/apache/pulsar/blob/39dd1cda2d01a6d472b7a39115a774958a837be1/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java#L746-L752
I don't _think_ this behavior is necessarily a bug. What I mean by that, is if my application takes >30s to process a message, then the ackTimeout setting kicks in and tells pulsar to redeliver it. Then the pulsar arrives to my application again (likely on a different host), and again takes >30s to redeliver. We aren't getting an opportunity to nack the message, but instead just causing pulsar to redeliver our message over & over. We observed this a lot on the 2.7.2 client, so maybe it has improved since then
--
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