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/01/28 21:13:07 UTC
[GitHub] [pulsar] congbobo184 opened a new pull request #14021: Fix cumulativeAckMode decrease unackMessageCount thow NPE
congbobo184 opened a new pull request #14021:
URL: https://github.com/apache/pulsar/pull/14021
link https://github.com/apache/pulsar/pull/13383
## Motivation
#13383 has fixed the batch message ack does not decrease the unacked-msg count, but in cumulative ack mode also decrease, it will use pendingAcks, but in cumulative ack, this will not init.
### Modifications
add judge `Subscription.isIndividualAckMode(subType)` when get ackCount.
### Verifying this change
Add the tests for it
Does this pull request potentially affect one of the following parts:
If yes was chosen, please highlight the changes
Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)
--
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] gaoran10 commented on a change in pull request #14021: Fix cumulativeAckMode decrease unackMessageCount thow NPE
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on a change in pull request #14021:
URL: https://github.com/apache/pulsar/pull/14021#discussion_r795005331
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BatchMessageWithBatchIndexLevelTest.java
##########
@@ -105,4 +111,61 @@ public void testBatchMessageAck() {
assertEquals(dispatcher.getConsumers().get(0).getUnackedMessages(), 16);
});
}
+
+ @DataProvider(name = "subType")
+ public Object[][] subType() {
+ return new Object[][] { { SubscriptionType.Shared }, { SubscriptionType.Failover } };
+ }
+
+
+ @Test(dataProvider="subType")
+ private void testDecreaseUnAckMessageCountWithAckReceipt(SubscriptionType subType) throws Exception {
+
+ final int messageCount = 50;
+ final String topicName = "persistent://prop/ns-abc/testDecreaseWithAckReceipt" + UUID.randomUUID();
+ final String subscriptionName = "sub-batch-1";
+ @Cleanup
+ ConsumerImpl<byte[]> consumer = (ConsumerImpl<byte[]>) pulsarClient
+ .newConsumer(Schema.BYTES)
+ .topic(topicName)
+ .isAckReceiptEnabled(true)
+ .subscriptionName(subscriptionName)
+ .subscriptionType(subType)
+ .isAckReceiptEnabled(true)
+ .subscribe();
+
+ @Cleanup
+ Producer<byte[]> producer = pulsarClient
+ .newProducer()
+ .topic(topicName)
+ .enableBatching(false)
Review comment:
Do we need to disable the batch?
--
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 #14021: Fix cumulativeAckMode decrease unackMessageCount thow NPE
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14021:
URL: https://github.com/apache/pulsar/pull/14021#issuecomment-1024640765
@congbobo184: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] mattisonchao commented on pull request #14021: Fix cumulativeAckMode decrease unackMessageCount thow NPE
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #14021:
URL: https://github.com/apache/pulsar/pull/14021#issuecomment-1024768841
@Technoboy- PTAL :)
--
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 pull request #14021: Fix cumulativeAckMode decrease unackMessageCount thow NPE
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #14021:
URL: https://github.com/apache/pulsar/pull/14021#issuecomment-1024825360
Unack messages for Failover and Exclusive subscription is meaningless.
--
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 #14021: Fix cumulativeAckMode decrease unackMessageCount thow NPE
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14021:
URL: https://github.com/apache/pulsar/pull/14021#issuecomment-1024640617
@congbobo184: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] Technoboy- commented on a change in pull request #14021: Fix NPE of cumulative ack mode and incorrect unack message count
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14021:
URL: https://github.com/apache/pulsar/pull/14021#discussion_r795050774
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java
##########
@@ -519,9 +521,10 @@ private long getBatchSize(MessageIdData msgId) {
return batchSize;
}
- private long getAckedCountForBatchIndexLevelEnabled(PositionImpl position, long batchSize, long[] ackSets) {
+ private long getAckedCountForBatchIndexLevelEnabled(PositionImpl position, long batchSize,
+ long[] ackSets, SubType subType) {
Review comment:
Seems that we don't need `subType`.
--
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 #14021: Fix cumulativeAckMode decrease unackMessageCount thow NPE
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14021:
URL: https://github.com/apache/pulsar/pull/14021#issuecomment-1024639633
@congbobo184: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] Technoboy- commented on a change in pull request #14021: Fix NPE of cumulative ack mode and incorrect unack message count
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14021:
URL: https://github.com/apache/pulsar/pull/14021#discussion_r795050834
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java
##########
@@ -519,9 +521,10 @@ private long getBatchSize(MessageIdData msgId) {
return batchSize;
}
- private long getAckedCountForBatchIndexLevelEnabled(PositionImpl position, long batchSize, long[] ackSets) {
+ private long getAckedCountForBatchIndexLevelEnabled(PositionImpl position, long batchSize,
+ long[] ackSets, SubType subType) {
long ackedCount = 0;
- if (isAcknowledgmentAtBatchIndexLevelEnabled) {
+ if (Subscription.isIndividualAckMode(subType) && isAcknowledgmentAtBatchIndexLevelEnabled) {
Review comment:
Suggest to change to if (isAcknowledgmentAtBatchIndexLevelEnabled && Subscription.isIndividualAckMode(subType) )
--
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] gaoran10 commented on a change in pull request #14021: Fix cumulativeAckMode decrease unackMessageCount thow NPE
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on a change in pull request #14021:
URL: https://github.com/apache/pulsar/pull/14021#discussion_r795005331
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BatchMessageWithBatchIndexLevelTest.java
##########
@@ -105,4 +111,61 @@ public void testBatchMessageAck() {
assertEquals(dispatcher.getConsumers().get(0).getUnackedMessages(), 16);
});
}
+
+ @DataProvider(name = "subType")
+ public Object[][] subType() {
+ return new Object[][] { { SubscriptionType.Shared }, { SubscriptionType.Failover } };
+ }
+
+
+ @Test(dataProvider="subType")
+ private void testDecreaseUnAckMessageCountWithAckReceipt(SubscriptionType subType) throws Exception {
+
+ final int messageCount = 50;
+ final String topicName = "persistent://prop/ns-abc/testDecreaseWithAckReceipt" + UUID.randomUUID();
+ final String subscriptionName = "sub-batch-1";
+ @Cleanup
+ ConsumerImpl<byte[]> consumer = (ConsumerImpl<byte[]>) pulsarClient
+ .newConsumer(Schema.BYTES)
+ .topic(topicName)
+ .isAckReceiptEnabled(true)
+ .subscriptionName(subscriptionName)
+ .subscriptionType(subType)
+ .isAckReceiptEnabled(true)
+ .subscribe();
+
+ @Cleanup
+ Producer<byte[]> producer = pulsarClient
+ .newProducer()
+ .topic(topicName)
+ .enableBatching(false)
Review comment:
Do we need to disable the batch? It seems that this configuration doesn't match the configuration `batchingMaxMessages`.
--
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] congbobo184 merged pull request #14021: Fix NPE of cumulative ack mode and incorrect unack message count
Posted by GitBox <gi...@apache.org>.
congbobo184 merged pull request #14021:
URL: https://github.com/apache/pulsar/pull/14021
--
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