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/11/29 04:26:53 UTC
[GitHub] [pulsar-client-cpp] BewareMyPower opened a new pull request, #128: [fix] Fix acknowledge MessageId list does not work when ackGroupingTimeMs is 0
BewareMyPower opened a new pull request, #128:
URL: https://github.com/apache/pulsar-client-cpp/pull/128
### Motivation
When `ackGroupingTimeMs` is 0, `AckGroupingTrackerDisabled` will be created to acknowledge messages immediately. However, since the `addAcknowledgeList` method is not implemented in #23, it doesn't work.
### Modifications
Implement `AckGroupingTrackerDisabled::addAcknowledgeList`. Move the protocol version validation from `AckGroupingTrackerEnabled` to the base class `AckGroupingTracker` so that the `doImmediateAck` overload that accepts `std::set<MessageId>` can be reused.
Add testing params to `testAckMsgList` and
`testAckMsgListWithMultiConsumer` so that various `ackGroupingTimeMs` can be verified.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `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-client-cpp] RobertIndie commented on a diff in pull request #128: [fix] Fix acknowledge MessageId list does not work when ackGroupingTimeMs is 0
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on code in PR #128:
URL: https://github.com/apache/pulsar-client-cpp/pull/128#discussion_r1035617704
##########
tests/BasicEndToEndTest.cc:
##########
@@ -4277,7 +4277,9 @@ TEST(BasicEndToEndTest, testBatchReceiveClose) { testBatchReceiveClose(false); }
TEST(BasicEndToEndTest, testBatchReceiveCloseWithMultiConsumer) { testBatchReceiveClose(true); }
-TEST(BasicEndToEndTest, testAckMsgList) {
+class AcknowledgeTest : public testing::TestWithParam<int> {};
Review Comment:
How about moving this test class to a separate file.
--
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-client-cpp] BewareMyPower commented on a diff in pull request #128: [fix] Fix acknowledge MessageId list does not work when ackGroupingTimeMs is 0
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #128:
URL: https://github.com/apache/pulsar-client-cpp/pull/128#discussion_r1035653396
##########
tests/BasicEndToEndTest.cc:
##########
@@ -4277,7 +4277,9 @@ TEST(BasicEndToEndTest, testBatchReceiveClose) { testBatchReceiveClose(false); }
TEST(BasicEndToEndTest, testBatchReceiveCloseWithMultiConsumer) { testBatchReceiveClose(true); }
-TEST(BasicEndToEndTest, testAckMsgList) {
+class AcknowledgeTest : public testing::TestWithParam<int> {};
Review Comment:
It make sense to me.
BTW, the `BasicEndToEndTest` is too huge, adding a test into this file requires much more compile time than others. The original purpose is to use another PR to split tests in this file. For this PR, I will split the test class to another file soon.
--
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-client-cpp] BewareMyPower commented on pull request #128: [fix] Fix acknowledge MessageId list does not work when ackGroupingTimeMs is 0
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #128:
URL: https://github.com/apache/pulsar-client-cpp/pull/128#issuecomment-1330063293
@RobertIndie Please also include this PR in 3.1.0 since the VOTE of candidate-2 is not present yet.
--
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-client-cpp] BewareMyPower merged pull request #128: [fix] Fix acknowledge MessageId list does not work when ackGroupingTimeMs is 0
Posted by GitBox <gi...@apache.org>.
BewareMyPower merged PR #128:
URL: https://github.com/apache/pulsar-client-cpp/pull/128
--
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