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/13 05:07:16 UTC
[GitHub] [pulsar] gaozhangmin opened a new pull request, #16033: Fix miss flushing ack request when invoking pendingIndividualBatchInd…
gaozhangmin opened a new pull request, #16033:
URL: https://github.com/apache/pulsar/pull/16033
### Motivation
#15877 missed another condition where `flush` should be invoked.
### Modifications
Flush ack request after `pendingIndividualBatchIndexAcks` size >= MAX_ACK_GROUP_SIZE when doing `addListAcknowledgment(messageIds)`
### Documentation
Check the box below or label this PR directly.
Need to update docs?
- [x] `doc-not-needed`
(Please explain why)
--
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 #16033: [fix][client]Fix miss flushing ack request when invoking pendingIndividualBatchAck
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on PR #16033:
URL: https://github.com/apache/pulsar/pull/16033#issuecomment-1156166927
When i was adding UT, i found there may exists another problem: Message ack in a batch will be added to `pendingIndividualBatchIndexAcks`, After the BitSet all set to false(means all acked, but the ack requests are not sent yet), a normal ack request will be send to broker, and the batch index ack will be removed from `pendingIndividualBatchIndexAcks` before we send it to broker. @gaoran10 @lhotari @codelipenghui
--
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 #16033: [fix][client]Fix miss flushing ack request when invoking pendingIndividualBatchAck
Posted by GitBox <gi...@apache.org>.
gaozhangmin closed pull request #16033: [fix][client]Fix miss flushing ack request when invoking pendingIndividualBatchAck
URL: https://github.com/apache/pulsar/pull/16033
--
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 #16033: [fix][client]Fix miss flushing ack request when invoking pendingIndividualBatchAck
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on PR #16033:
URL: https://github.com/apache/pulsar/pull/16033#issuecomment-1154188041
Maybe we'd better add this inspection at where the `pendingIndividualBatchIndexAcks` changed, otherwise if there is a new method to call the method `doIndividualBatchAckAsync`, it also needs to add this inspection.
--
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] lhotari commented on a diff in pull request #16033: [fix][client]Fix miss flushing ack request when invoking pendingIndividualBatchAck
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #16033:
URL: https://github.com/apache/pulsar/pull/16033#discussion_r896447899
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java:
##########
@@ -312,15 +312,9 @@ private CompletableFuture<Void> doIndividualBatchAck(BatchMessageIdImpl batchMes
return this.currentIndividualAckFuture;
} finally {
this.lock.readLock().unlock();
- if (pendingIndividualBatchIndexAcks.size() >= MAX_ACK_GROUP_SIZE) {
- flush();
- }
}
} else {
doIndividualBatchAckAsync(batchMessageId);
- if (pendingIndividualBatchIndexAcks.size() >= MAX_ACK_GROUP_SIZE) {
- flush();
- }
Review Comment:
This change doesn't match the PR description.
--
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 #16033: [fix][client]Fix miss flushing ack request when invoking pendingIndividualBatchAck
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on PR #16033:
URL: https://github.com/apache/pulsar/pull/16033#issuecomment-1162662289
It's impossible that flush is trigger by `pendingIndividualBatchIndexAcks` size. according to he above description.
--
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