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/07/29 17:44:15 UTC
[GitHub] [pulsar] 315157973 opened a new pull request #7688: Expose MessagesImpl
315157973 opened a new pull request #7688:
URL: https://github.com/apache/pulsar/pull/7688
Fixes #7626
### Motivation
Expose `MessagesImpl` ，so that we can ack list of messages by using `CompletableFuture<Void> acknowledgeAsync(Messages<?> messages)`
### Modifications
Change the visibility level of the method from protect to public
### Verifying this change
unit test:
org.apache.pulsar.client.api.ConsumerBatchReceiveTest#testBatchAck
----------------------------------------------------------------
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] 315157973 commented on pull request #7688: Expose MessagesImpl
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #7688:
URL: https://github.com/apache/pulsar/pull/7688#issuecomment-666887157
Requirements:
Receive a batch of messages (BatchA), but only ack some of them (BatchB), and only call one API to ack BatchB
Solution：
Change the visibility level of the method from protect to public,so that we can ack list of messages by using `acknowledge(Messages<?> messages)`
Suggestion：
1)Since the internal implementation of the `acknowledge(Messages<?> messages)` method is still a circular ack, merlimat believes that there is no performance improvement.
2)codelipenghui believes that acknowledge (Messages<?> messages) should not be reused, because Messages has restrictions, and `acknowledge(List<MessageIds> msgIds)` should be added。Although I prefer to use Messages。
Solution2：
1) add method `acknowledge(List<MessageId> msgIds)`。
2) add batch individual acknowledgment to `PersistentAcknowledgmentsGroupingTracker`, ack a batch of messages only requires one request instead of circular requests
@merlimat @codelipenghui @sijie
----------------------------------------------------------------
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 edited a comment on pull request #7688: Expose MessagesImpl
Posted by GitBox <gi...@apache.org>.
merlimat edited a comment on pull request #7688:
URL: https://github.com/apache/pulsar/pull/7688#issuecomment-665843163
I don't understand the reason for this change. there's no performance improvement in this versus just acking the individual messages
----------------------------------------------------------------
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] pg2404 commented on pull request #7688: Expose MessagesImpl
Posted by GitBox <gi...@apache.org>.
pg2404 commented on pull request #7688:
URL: https://github.com/apache/pulsar/pull/7688#issuecomment-666356107
Thank you @codelipenghui.
@merlimat - there is a lot of scope of performance improvement in the scenario I described - where I am receiving a batch of messages, grouping them and processing them - and only on successful processing of the batch (or even partial success), would need to send ack - by the time I am in a position to send an ack/negative ack I have lost the ordering of message ids and cumulativeAck will not work (I might need some of the intermediate messages redelivered) - Right now I am having to group these messages and loop over these groups to send ack - with this change I can maintain the message Ids successfully processed and fire a single async call with the list.
----------------------------------------------------------------
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 #7688: Expose MessagesImpl
Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #7688:
URL: https://github.com/apache/pulsar/pull/7688#issuecomment-665843163
I don't understand the reason for this change. there's performance improvement in this versus just acking the individual messages
----------------------------------------------------------------
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] pg2404 commented on pull request #7688: Expose MessagesImpl
Posted by GitBox <gi...@apache.org>.
pg2404 commented on pull request #7688:
URL: https://github.com/apache/pulsar/pull/7688#issuecomment-666558755
@sijie - I was only looking for suggestions to implement my use case really - any guidance would be helpful even if a new API is not exposed. My basic pain point being, even if I do a receive a batch - I cannot guarantee that the entire batch will fail or pass in the single shot - I have to group the data on certain metadata and divide the messages into smaller batches and write them somewhere - if the entire process succeeds for the entire batch only then can I send the ack. Suppose, out of the batch I have 4 sub groups and out of which 2 fails I cannot ack the batch - the ack has to be done per message.
----------------------------------------------------------------
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 #7688: Support acknowledging a list of messages
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #7688:
URL: https://github.com/apache/pulsar/pull/7688#issuecomment-674100197
/pulsarbot run-failure-checks
----------------------------------------------------------------
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] 315157973 commented on pull request #7688: Expose MessagesImpl
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #7688:
URL: https://github.com/apache/pulsar/pull/7688#issuecomment-667634366
Please review @merlimat @sijie
----------------------------------------------------------------
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] sijie commented on pull request #7688: Expose MessagesImpl
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #7688:
URL: https://github.com/apache/pulsar/pull/7688#issuecomment-666544372
@pg2404 I understand the motivation. However, I am not sure we are implementing this in the right way.
In your use case, can't you use batchReceive to receive a batch of messages and ack the whole batch?
If we want to enhance the API to support batch acknowledgment, doesn't it make more sense to support
```
ack(List<Message> msgs);
ack(List<Messages> msgBatches);
```
----------------------------------------------------------------
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 merged pull request #7688: Support acknowledging a list of messages
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #7688:
URL: https://github.com/apache/pulsar/pull/7688
----------------------------------------------------------------
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