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 2021/02/14 08:44:06 UTC
[GitHub] [pulsar] MarvinCai opened a new pull request #9583: Disable negativeAck for Key_Shared sub type.
MarvinCai opened a new pull request #9583:
URL: https://github.com/apache/pulsar/pull/9583
Fixes #9582
### Motivation
The message negative ack will break the message ordering dispatch guarantee, Key_Shared is an ordering dispatch guarantee subscription, so we should avoid the message negative ack for the Key_Shared subscription.
### Modifications
Check if it's Key_Shared subscription type before negativeAcking messages.
### Verifying this change
This change added tests and can be verified as follows:
- *Added test case.*
----------------------------------------------------------------
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] eolivelli commented on a change in pull request #9583: Disable negativeAck for Key_Shared sub type.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9583:
URL: https://github.com/apache/pulsar/pull/9583#discussion_r575797734
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java
##########
@@ -672,10 +673,17 @@ public UnAckedMessageTracker getUnAckedMessageTracker() {
@Override
public void negativeAcknowledge(MessageId messageId) {
- negativeAcksTracker.add(messageId);
+ if (getSubType() != SubType.Key_Shared) {
+ negativeAcksTracker.add(messageId);
- // Ensure the message is not redelivered for ack-timeout, since we did receive an "ack"
- unAckedMessageTracker.remove(messageId);
+ // Ensure the message is not redelivered for ack-timeout, since we did receive an "ack"
+ unAckedMessageTracker.remove(messageId);
+ } else {
+ if (log.isDebugEnabled()) {
+ log.debug("NegativeAcknowledge for Key_Shared subscription type for message {} is ignored as this will" +
Review comment:
Why aren't we throwing an error?
----------------------------------------------------------------
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 pull request #9583: Disable negativeAck for Key_Shared sub type.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #9583:
URL: https://github.com/apache/pulsar/pull/9583#issuecomment-783424273
I see, thanks for the explanation. So for the DLQ and negative ack, we can allow to enable them for all subscription types but if the purpose is ordering, we should not use DLQ and 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
[GitHub] [pulsar] MarvinCai closed pull request #9583: Disable negativeAck for Key_Shared sub type.
Posted by GitBox <gi...@apache.org>.
MarvinCai closed pull request #9583:
URL: https://github.com/apache/pulsar/pull/9583
----------------------------------------------------------------
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 pull request #9583: Disable negativeAck for Key_Shared sub type.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #9583:
URL: https://github.com/apache/pulsar/pull/9583#issuecomment-779543465
@merlimat
I'm not sure how `the negative acks also work for Exclusive and Failover subscriptions, which are also ordered` works, from the source code, seems nothing different for the negative ack between different subscription types. For sequential processing, the application usually processes the next message after one message is successfully processed. So the application should retry the failed message until processed successfully.
Currently, users can use the strictly ordered guarantee and allow out of order. I think If users enabled the out of order option, they can use the DLQ/nack, but for the strictly ordered guarantee, we should avoid the DLQ and nack.
----------------------------------------------------------------
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] merlimat commented on pull request #9583: Disable negativeAck for Key_Shared sub type.
Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #9583:
URL: https://github.com/apache/pulsar/pull/9583#issuecomment-779949440
My point of comparison was that it doesn't make sense to disable for key-shared when it's allowed for exclusive and failover.
The bigger point here is that the exclusiveness is not only done with the purpose of ordering (both for exclusive, failover as well as for key-shared type).
----------------------------------------------------------------
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] MarvinCai commented on pull request #9583: Disable negativeAck for Key_Shared sub type.
Posted by GitBox <gi...@apache.org>.
MarvinCai commented on pull request #9583:
URL: https://github.com/apache/pulsar/pull/9583#issuecomment-783870350
As discussed, closing this pr, will have another pr to update document for negative ack/DLQ for ordered sub type.
----------------------------------------------------------------
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