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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
