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 2020/05/04 11:37:32 UTC
[GitHub] [pulsar] lhotari opened a new issue #6869: Negative acknowledgement doesn't remove the message id from UnAckedMessageTracker when message id is instance of BatchMessageIdImpl
lhotari opened a new issue #6869:
URL: https://github.com/apache/pulsar/issues/6869
**Describe the bug**
The actual symptom was that when using the DLQ feature, the redelivery counts were not consistent in a use case where negative acknowledgements are used. Messages would get redelivered more times than the configured maxRedeliverCount on the DeadLetterPolicy.
I observed this type of log messages in the log output:
```
14:20:07.080 [pulsar-timer-4-1] WARN o.a.p.c.impl.UnAckedMessageTracker - [ConsumerBase{subscription='Test-Subscriber', consumerName='f194e', topic='test-topic'}] 5 messages have timed-out
```
By debugging, I noticed that calling `org.apache.pulsar.client.api.Consumer#negativeAcknowledge(org.apache.pulsar.client.api.MessageId)` doesn't remove the message id from `UnAckedMessageTracker` when the message is instance of BatchMessageIdImpl.
**Expected behavior**
org.apache.pulsar.client.impl.UnAckedMessageTracker implementation should encapsulate the fact that the message id must be MessageIdImpl and not BatchMessageIdImpl.
Currently the logic to first convert a BatchMessageIdImpl is done on the calling side (examples: https://github.com/apache/pulsar/blob/de57ddd572dbc74529a56fee68c6be37bd35cf7c/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java#L521-L531 , https://github.com/apache/pulsar/blob/de57ddd572dbc74529a56fee68c6be37bd35cf7c/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java#L1155-L1164 )
Since the caller has to convert the message id before calling UnAckedMessageTracker add or remove methods, it seems that this leads to error prone usage of the UnAckedMessageTracker class. Currently the conversion to MessageIdImpl is missing in the negative acknowledgement method:
https://github.com/apache/pulsar/blob/de57ddd572dbc74529a56fee68c6be37bd35cf7c/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java#L556-L557
**Work around**
One workaround is to convert a possible BatchMessageIdImpl to MessageIdImpl before calling the `negativeAcknowledge` method. Something like this
```java
...
consumer.negativeAcknowledge(convertMessageIdForNack(message.getMessageId()));
...
// workaround Pulsar bug regarding negative acknowledgements
private static MessageId convertMessageIdForNack(MessageId messageId) {
if (messageId instanceof BatchMessageIdImpl) {
// use similar logic as there is in org.apache.pulsar.client.impl.NegativeAcksTracker#add
BatchMessageIdImpl batchMessageId = (BatchMessageIdImpl) messageId;
return new MessageIdImpl(batchMessageId.getLedgerId(), batchMessageId.getEntryId(),
batchMessageId.getPartitionIndex());
} else {
return messageId;
}
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wolfstudy commented on issue #6869: Negative acknowledgement doesn't remove the message id from UnAckedMessageTracker when message id is instance of BatchMessageIdImpl
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on issue #6869:
URL: https://github.com/apache/pulsar/issues/6869#issuecomment-664801766
Move the task to 2.6.2
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on issue #6869: Negative acknowledgement doesn't remove the message id from UnAckedMessageTracker when message id is instance of BatchMessageIdImpl
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on issue #6869:
URL: https://github.com/apache/pulsar/issues/6869#issuecomment-697062213
@lhotari Sorry, I missed your last comment. We will try to fix it on 2.6.2.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on issue #6869: Negative acknowledgement doesn't remove the message id from UnAckedMessageTracker when message id is instance of BatchMessageIdImpl
Posted by GitBox <gi...@apache.org>.
lhotari commented on issue #6869:
URL: https://github.com/apache/pulsar/issues/6869#issuecomment-640724523
Hi @codelipenghui , I noticed on Twitter a notice that the release for 2.6.0 will be cut soon. Would it be possible to include a fix in 2.6.0 for this nack & BatchMessageImpl 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on issue #6869: Negative acknowledgement doesn't remove the message id from UnAckedMessageTracker when message id is instance of BatchMessageIdImpl
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on issue #6869:
URL: https://github.com/apache/pulsar/issues/6869#issuecomment-626443716
@lhotari Yes, the problem is related to batch index acknowledgment. We will advance merge #6052. I think this issue can be fixed by enabling batch index acknowledgment.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on issue #6869: Negative acknowledgement doesn't remove the message id from UnAckedMessageTracker when message id is instance of BatchMessageIdImpl
Posted by GitBox <gi...@apache.org>.
lhotari commented on issue #6869:
URL: https://github.com/apache/pulsar/issues/6869#issuecomment-623414917
I wonder if issue #5969 is related.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] devinbost commented on issue #6869: Negative acknowledgement doesn't remove the message id from UnAckedMessageTracker when message id is instance of BatchMessageIdImpl
Posted by GitBox <gi...@apache.org>.
devinbost commented on issue #6869:
URL: https://github.com/apache/pulsar/issues/6869#issuecomment-857055751
Any updates on this?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on issue #6869: Negative acknowledgement doesn't remove the message id from UnAckedMessageTracker when message id is instance of BatchMessageIdImpl
Posted by GitBox <gi...@apache.org>.
lhotari commented on issue #6869:
URL: https://github.com/apache/pulsar/issues/6869#issuecomment-637292781
> @lhotari Yes, the problem is related to batch index acknowledgment. We will advance merge #6052. I think this issue can be fixed by enabling batch index acknowledgment.
@codelipenghui It seems that #6052 was merged to master, however there doesn't seem to be any new logic to properly handle negative acknowledgements when the message id is instance of BatchMessageIdImpl. Perhaps I missed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on issue #6869: Negative acknowledgement doesn't remove the message id from UnAckedMessageTracker when message id is instance of BatchMessageIdImpl
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on issue #6869:
URL: https://github.com/apache/pulsar/issues/6869#issuecomment-697062213
@lhotari Sorry, I missed your last comment. We will try to fix it on 2.6.2.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari edited a comment on issue #6869: Negative acknowledgement doesn't remove the message id from UnAckedMessageTracker when message id is instance of BatchMessageIdImpl
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on issue #6869:
URL: https://github.com/apache/pulsar/issues/6869#issuecomment-637292781
> @lhotari Yes, the problem is related to batch index acknowledgment. We will advance merge #6052. I think this issue can be fixed by enabling batch index acknowledgment.
@codelipenghui It seems that #6052 was merged to master, however there doesn't seem to be any new logic to properly handle negative acknowledgements when the message id is instance of BatchMessageIdImpl. Perhaps I missed it when I tried to find the way it's 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on issue #6869: Negative acknowledgement doesn't remove the message id from UnAckedMessageTracker when message id is instance of BatchMessageIdImpl
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on issue #6869:
URL: https://github.com/apache/pulsar/issues/6869#issuecomment-637341326
@lhotari Thanks for your feedback. Looks #6052 just can avoid the acked messages in the batch message deliver to the consumer. We need to find a way to handle negative acknowledgment. We need to remove the message ID from the unack message tracker and send the redelivery request to the broker until all batch indexes of the batch message are processed(ack or negative ack).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org