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/06/01 09:46:03 UTC
[GitHub] [pulsar] gaozhangmin opened a new pull request, #15877: [fix][clients]Check pendingIndividualBatchIndexAcks size in doIndividualBatchAckAsync
gaozhangmin opened a new pull request, #15877:
URL: https://github.com/apache/pulsar/pull/15877
### Motivation
Now， We check pendingIndividualBatchIndexAcks then flush in method `doCumulativeAck`,
But We only insert data to pendingIndividualBatchIndexAcks when `doIndividualBatchAckAsync`
### Modifications
Remove pendingIndividualBatchIndexAcks in method `doCumulativeAck`
Execute check pendingIndividualBatchIndexAcks in method `doIndividualBatchAckAsync`.
### Documentation
Check the box below or label this PR directly.
Need to update docs?
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [ ] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
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 diff in pull request #15877: [fix][clients]Check pendingIndividualBatchIndexAcks size in doIndividualBatchAckAsync
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15877:
URL: https://github.com/apache/pulsar/pull/15877#discussion_r894123677
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java:
##########
@@ -366,6 +360,9 @@ private void doIndividualBatchAckAsync(BatchMessageIdImpl batchMessageId) {
return value;
});
bitSet.clear(batchMessageId.getBatchIndex());
+ if (pendingIndividualBatchIndexAcks.size() >= MAX_ACK_GROUP_SIZE) {
+ flush();
+ }
Review Comment:
> `doIndividualBatchAck` also invokes `doIndividualBatchAckAsync`, what's the difference?
Maybe the name of the method is a little inappropriate.
--
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] gaozhangmin commented on pull request #15877: [fix][clients]Check pendingIndividualBatchIndexAcks size in doIndividualBatchAckAsync
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on PR #15877:
URL: https://github.com/apache/pulsar/pull/15877#issuecomment-1151939719
> Could you try to add a unit test to cover this logic?
I believe it already covered by `testDoIndividualBatchAckAsync`
--
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 diff in pull request #15877: [fix][clients]Check pendingIndividualBatchIndexAcks size in doIndividualBatchAckAsync
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on code in PR #15877:
URL: https://github.com/apache/pulsar/pull/15877#discussion_r895280149
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java:
##########
@@ -366,6 +360,9 @@ private void doIndividualBatchAckAsync(BatchMessageIdImpl batchMessageId) {
return value;
});
bitSet.clear(batchMessageId.getBatchIndex());
+ if (pendingIndividualBatchIndexAcks.size() >= MAX_ACK_GROUP_SIZE) {
+ flush();
+ }
Review Comment:
It seems that there is also a method `PersistentAcknowledgmentsGroupingTracker#addListAcknowledgment` that may increase the pending individual count.
--
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] BewareMyPower commented on a diff in pull request #15877: [fix][clients]Check pendingIndividualBatchIndexAcks size in doIndividualBatchAckAsync
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #15877:
URL: https://github.com/apache/pulsar/pull/15877#discussion_r894098275
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java:
##########
@@ -366,6 +360,9 @@ private void doIndividualBatchAckAsync(BatchMessageIdImpl batchMessageId) {
return value;
});
bitSet.clear(batchMessageId.getBatchIndex());
+ if (pendingIndividualBatchIndexAcks.size() >= MAX_ACK_GROUP_SIZE) {
+ flush();
+ }
Review Comment:
`doIndividualBatchAck` also invokes `doIndividualBatchAckAsync`, what's the difference?
--
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 diff in pull request #15877: [fix][clients]Check pendingIndividualBatchIndexAcks size in doIndividualBatchAckAsync
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on code in PR #15877:
URL: https://github.com/apache/pulsar/pull/15877#discussion_r894105483
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java:
##########
@@ -366,6 +360,9 @@ private void doIndividualBatchAckAsync(BatchMessageIdImpl batchMessageId) {
return value;
});
bitSet.clear(batchMessageId.getBatchIndex());
+ if (pendingIndividualBatchIndexAcks.size() >= MAX_ACK_GROUP_SIZE) {
+ flush();
+ }
Review Comment:
Maybe we could add the logic here because only this method could add entries to `pendingIndividualBatchIndexAcks`.
--
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] gaozhangmin commented on pull request #15877: [fix][clients]Check pendingIndividualBatchIndexAcks size in doIndividualBatchAckAsync
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on PR #15877:
URL: https://github.com/apache/pulsar/pull/15877#issuecomment-1144401412
@mattisonchao @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] gaozhangmin merged pull request #15877: [fix][clients]Check pendingIndividualBatchIndexAcks size in doIndividualBatchAckAsync
Posted by GitBox <gi...@apache.org>.
gaozhangmin merged PR #15877:
URL: https://github.com/apache/pulsar/pull/15877
--
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 diff in pull request #15877: [fix][clients]Check pendingIndividualBatchIndexAcks size in doIndividualBatchAckAsync
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15877:
URL: https://github.com/apache/pulsar/pull/15877#discussion_r894089217
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java:
##########
@@ -366,6 +360,9 @@ private void doIndividualBatchAckAsync(BatchMessageIdImpl batchMessageId) {
return value;
});
bitSet.clear(batchMessageId.getBatchIndex());
+ if (pendingIndividualBatchIndexAcks.size() >= MAX_ACK_GROUP_SIZE) {
+ flush();
+ }
Review Comment:
Following the current code logic, this code is better placed in PersistentAcknowledgmentsGroupingTracker#doIndividualBatchAck(BatchMessageIdImpl batchMessageId)
--
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 #15877: [fix][clients]Check pendingIndividualBatchIndexAcks size in doIndividualBatchAckAsync
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15877:
URL: https://github.com/apache/pulsar/pull/15877#issuecomment-1143447421
@gaozhangmin: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] gaoran10 commented on pull request #15877: [fix][clients]Check pendingIndividualBatchIndexAcks size in doIndividualBatchAckAsync
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on PR #15877:
URL: https://github.com/apache/pulsar/pull/15877#issuecomment-1153388614
It seems that there is also a method PersistentAcknowledgmentsGroupingTracker#addListAcknowledgment that may increase the pending individual count.
--
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] BewareMyPower commented on a diff in pull request #15877: [fix][clients]Check pendingIndividualBatchIndexAcks size in doIndividualBatchAckAsync
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #15877:
URL: https://github.com/apache/pulsar/pull/15877#discussion_r894153358
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java:
##########
@@ -366,6 +360,9 @@ private void doIndividualBatchAckAsync(BatchMessageIdImpl batchMessageId) {
return value;
});
bitSet.clear(batchMessageId.getBatchIndex());
+ if (pendingIndividualBatchIndexAcks.size() >= MAX_ACK_GROUP_SIZE) {
+ flush();
+ }
Review Comment:
> we check pendingIndividualAcks in the doIndividualAck, so we should keep the same code logic.
It makes sense to me.
--
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] gaozhangmin closed pull request #15877: [fix][clients]Check pendingIndividualBatchIndexAcks size in doIndividualBatchAckAsync
Posted by GitBox <gi...@apache.org>.
gaozhangmin closed pull request #15877: [fix][clients]Check pendingIndividualBatchIndexAcks size in doIndividualBatchAckAsync
URL: https://github.com/apache/pulsar/pull/15877
--
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 diff in pull request #15877: [fix][clients]Check pendingIndividualBatchIndexAcks size in doIndividualBatchAckAsync
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15877:
URL: https://github.com/apache/pulsar/pull/15877#discussion_r894124487
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java:
##########
@@ -366,6 +360,9 @@ private void doIndividualBatchAckAsync(BatchMessageIdImpl batchMessageId) {
return value;
});
bitSet.clear(batchMessageId.getBatchIndex());
+ if (pendingIndividualBatchIndexAcks.size() >= MAX_ACK_GROUP_SIZE) {
+ flush();
+ }
Review Comment:
> Maybe we could add the logic here because only this method could add entries to `pendingIndividualBatchIndexAcks`.
Yes, I know. I mean: we check `pendingIndividualAcks` in the `doIndividualAck`, so we should keep the same code logic.
--
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] gaozhangmin commented on pull request #15877: [fix][clients]Check pendingIndividualBatchIndexAcks size in doIndividualBatchAckAsync
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on PR #15877:
URL: https://github.com/apache/pulsar/pull/15877#issuecomment-1153472860
> It seems that there is also a method PersistentAcknowledgmentsGroupingTracker#addListAcknowledgment that may increase the pending individual count.
I see, maybe the best place where to invoke is `doIndividualBatchAckAsync` like what i did before.
--
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