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/11/18 07:54:08 UTC
[GitHub] [pulsar] saosir opened a new pull request #8606: fix cpp client AcknowledgeCumulative
saosir opened a new pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606
pulsar-client-cpp Consumer UnAckedMessageTrackerEnabled::removeMessagesTill should not erase last msgId in messageIdPartitionMap
<!--
### Contribution Checklist
- Name the pull request in the form "[Issue XYZ][component] Title of the pull request", where *XYZ* should be replaced by the actual issue number.
Skip *Issue XYZ* if there is no associated github issue for this pull request.
Skip *component* if you are unsure about which is the best component. E.g. `[docs] Fix typo in produce method`.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
**(The sections below can be removed for hotfixes of typos)**
-->
*(If this PR fixes a github issue, please add `Fixes #<xyz>`.)*
*(or if this PR is one task of a github issue, please add `Master Issue: #<xyz>` to link to the master issue.)*
### Motivation
pulsar-client-cpp Consumer UnAckedMessageTrackerEnabled::removeMessagesTill should not erase last msgId in messageIdPartitionMap
### Modifications
erase msgIdInMap earlier than param `msgId` in `UnAckedMessageTrackerEnabled::removeMessagesTill`
### 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)
### Documentation
- Does this pull request introduce a new feature? (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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] saosir edited a comment on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir edited a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735593147
@BewareMyPower I have two questions here, can you help me answer them?
1. Both `PartitionedConsumerImpl` and `ConsumerImpl` have member variable `unAckedMessageTrackerPtr_` (class `UnAckedMessageTrackerEnabled`), and `PartitionedConsumerImpl` is composed of `ConsumerImpl`. if the acknowledgement times out, will they send redeliverMessages repeatedly?
2. in `UnAckedMessageTrackerEnabled`, param msg of `UnAckedMessageTrackerEnabled::add` contain `partition`, `ledgerId`, `entryId`,`batchIndex`. When ack timeout and do `redeliverUnacknowledgedMessages`, does it do deduplicte based on (`ledgerId`, `entryId`)? If not it will send repeat `ack(ledgerId, entryId)`
----------------------------------------------------------------
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] saosir commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-750729469
@BewareMyPower the `CI - Integration` is failed, can you help?
----------------------------------------------------------------
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] BewareMyPower commented on a change in pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#discussion_r532719627
##########
File path: pulsar-client-cpp/lib/UnAckedMessageTrackerEnabled.h
##########
@@ -25,25 +25,27 @@
namespace pulsar {
class UnAckedMessageTrackerEnabled : public UnAckedMessageTrackerInterface {
public:
- ~UnAckedMessageTrackerEnabled();
+ virtual ~UnAckedMessageTrackerEnabled() { this->close(); }
Review comment:
It won't pass the clang-format check here. Please use clang-format 5.0 to reformat your code.
----------------------------------------------------------------
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] saosir commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-751194394
@BewareMyPower `CI - Integration - Cli / cli (pull_request)` fail, see #8790. Do I need to rebase to master branch?
----------------------------------------------------------------
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] saosir removed a comment on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir removed a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-736460144
@BewareMyPower
why use `std::unique_lock` in `UnAckedMessageTrackerEnabled` , see
https://github.com/apache/pulsar/blob/91e2f832178d9ffd5d78161145d895910296c2d9/pulsar-client-cpp/lib/UnAckedMessageTrackerEnabled.cc#L42
but use `std::lock_guard` in `AckGroupingTrackerEnabled`
https://github.com/apache/pulsar/blob/91e2f832178d9ffd5d78161145d895910296c2d9/pulsar-client-cpp/lib/AckGroupingTrackerEnabled.cc#L149
----------------------------------------------------------------
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] saosir edited a comment on pull request #8606: fix cpp client AcknowledgeCumulative
Posted by GitBox <gi...@apache.org>.
saosir edited a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-731891030
@BewareMyPower Sorry，I did not describe clearly. `UnAckedMessageTrackerEnabled::removeMessagesTill` should erase message whose id <= `msgId` in `messageIdPartitionMap`
see [https://github.com/apache/pulsar/pull/8606/files](https://github.com/apache/pulsar/pull/8606/files), the origin implementation of cpp is different from that of Java client. cpp client just earse `msgId`, not <= `msgId`
----------------------------------------------------------------
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] BewareMyPower commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-737310986
/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] saosir edited a comment on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir edited a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735593147
1. Both `PartitionedConsumerImpl` and `ConsumerImpl` have member variable `unAckedMessageTrackerPtr_` (class `UnAckedMessageTrackerEnabled`), and `PartitionedConsumerImpl` is composed of `ConsumerImpl`. if the acknowledgement times out, will they send redeliverMessages repeatedly?
2. in `UnAckedMessageTrackerEnabled`, param msg of `UnAckedMessageTrackerEnabled::add` contain `partition`, `ledgerId`, `entryId`,`batchIndex`. When ack timeout and do `redeliverUnacknowledgedMessages`, does it deduplicte based on ledger and entryId?
----------------------------------------------------------------
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] BewareMyPower commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735525406
> @BewareMyPower Can you review this pull request again?
OK
----------------------------------------------------------------
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] saosir removed a comment on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir removed a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735580816
`PartitionedConsumerImpl` and `ConsumerImpl` have same member `unAckedMessageTrackerPtr_` (`PartitionedConsumerImpl` is composed of `ConsumerImpl`), if the acknowledgement times out, will they send redeliverMessages repeatedly？
----------------------------------------------------------------
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] saosir edited a comment on pull request #8606: fix cpp client AcknowledgeCumulative
Posted by GitBox <gi...@apache.org>.
saosir edited a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-731891030
@BewareMyPower Sorry，I did not describe clearly. `UnAckedMessageTrackerEnabled::removeMessagesTill` should erase message whose id <= `msgId` in `messageIdPartitionMap`
see [https://github.com/apache/pulsar/pull/8606/files](https://github.com/apache/pulsar/pull/8606/files), the origin implementation of cpp is different from that of Java client. cpp client just earse `msgId` (function param `msgId`), not <= `msgId`
----------------------------------------------------------------
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] BewareMyPower commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735682181
@saosir It's pleasure to have your contribution.
By the way, I just looked at the `UnAckedMessageTrackerEnabled` again. There's something to correct.
When you created a consumer with unacked messages tracker enabled like:
```c++
Consumer consumer;
ConsumerConfiguration consumerConf;
consumerConf.setUnAckedMessagesTimeoutMs(11000); // must >= 10000
consumerConf.setTickDurationInMs(11000);
Result result = client.subscribe("my-topic", "consumer-1", consumerConf, consumer);
```
If `my-topic` has N partitions, N+1 `UnAckedMessageTrackerEnabled` will be created. However, **only the tracker of `PartitionedConsumerImpl` will add message id.** Because when the internal consumers were created, the listener was set with the partitioned consumer's method, see https://github.com/apache/pulsar/blob/102fa9de03509b86e47f58ab8e1c0dde2095da3b/pulsar-client-cpp/lib/PartitionedConsumerImpl.cc#L231-L232
And `ConsumerImpl::messageReceived` wouldn't be called, which means `ConsumerImpl#notifyPendingReceivedCallback` and `ConsumerImpl#internalListener` wouldn't be called.
Therefore, the trackers of sub-consumers only do the redelivery but not add any message id, see https://github.com/apache/pulsar/blob/102fa9de03509b86e47f58ab8e1c0dde2095da3b/pulsar-client-cpp/lib/PartitionedConsumerImpl.cc#L497-L499
`redeliverUnacknowledgedMessages(messageIds)` for shared and key shared subscriptions, and `redeliverUnacknowledgedMessages()` for other subscriptions.
----------------------------------------------------------------
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] saosir removed a comment on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir removed a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735531567
I have some other problem fix codes and test cases that need to be updated. I will submit the update later. @BewareMyPower
----------------------------------------------------------------
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] saosir commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-736158268
I reset branch at first commit
----------------------------------------------------------------
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] saosir removed a comment on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir removed a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-751371353
@BewareMyPower all is done
----------------------------------------------------------------
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] saosir commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-736460144
@BewareMyPower
why use `std::unique_lock` in `UnAckedMessageTrackerEnabled.cc` , see
https://github.com/apache/pulsar/blob/91e2f832178d9ffd5d78161145d895910296c2d9/pulsar-client-cpp/lib/UnAckedMessageTrackerEnabled.cc#L42
but use `std::lock_guard` in `AckGroupingTrackerEnabled`
https://github.com/apache/pulsar/blob/91e2f832178d9ffd5d78161145d895910296c2d9/pulsar-client-cpp/lib/AckGroupingTrackerEnabled.cc#L149
----------------------------------------------------------------
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] saosir edited a comment on pull request #8606: fix cpp client AcknowledgeCumulative
Posted by GitBox <gi...@apache.org>.
saosir edited a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-731891030
@BewareMyPower Sorry，I did not describe clearly. UnAckedMessageTrackerEnabled::removeMessagesTill should erase message whose id <= msgId in `messageIdPartitionMap`
see [https://github.com/apache/pulsar/pull/8606/files](https://github.com/apache/pulsar/pull/8606/files), the origin implementation of cpp is different from that of Java client. cpp client just earse `msgId`, not <= `msgId`
----------------------------------------------------------------
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] BewareMyPower commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735608152
@saosir
From my perspective, both you concerned are right. But the Java client has the same behavior, like `MultiTopicsConsumerImpl` and `ConsumerImpl` both hold an `UnAckedMessageTracker` instance. Except that Java client supports chucked messages but C++ client doesn't.
----------------------------------------------------------------
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] saosir edited a comment on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir edited a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-736460144
@BewareMyPower
why use `std::unique_lock` in `UnAckedMessageTrackerEnabled` , see
https://github.com/apache/pulsar/blob/91e2f832178d9ffd5d78161145d895910296c2d9/pulsar-client-cpp/lib/UnAckedMessageTrackerEnabled.cc#L42
but use `std::lock_guard` in `AckGroupingTrackerEnabled`
https://github.com/apache/pulsar/blob/91e2f832178d9ffd5d78161145d895910296c2d9/pulsar-client-cpp/lib/AckGroupingTrackerEnabled.cc#L149
----------------------------------------------------------------
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] BewareMyPower commented on a change in pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#discussion_r532723610
##########
File path: pulsar-client-cpp/lib/UnAckedMessageTrackerEnabled.cc
##########
@@ -158,9 +158,15 @@ void UnAckedMessageTrackerEnabled::clear() {
}
}
-UnAckedMessageTrackerEnabled::~UnAckedMessageTrackerEnabled() {
- if (timer_) {
- timer_->cancel();
+void UnAckedMessageTrackerEnabled::start() {
+ this->scheduleTimer();
+}
+
+void UnAckedMessageTrackerEnabled::close() {
+ std::lock_guard<std::mutex> acquire(mutexTimer_);
+ if (this->timer_) {
+ boost::system::error_code ec;
+ this->timer_.reset();
Review comment:
Why remove the `cancel()` here?
----------------------------------------------------------------
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] saosir commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735648157
@BewareMyPower
I will submit a fix and test case for this issue later, and help review it
----------------------------------------------------------------
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] saosir commented on a change in pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir commented on a change in pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#discussion_r533014296
##########
File path: pulsar-client-cpp/lib/UnAckedMessageTrackerEnabled.cc
##########
@@ -158,9 +158,15 @@ void UnAckedMessageTrackerEnabled::clear() {
}
}
-UnAckedMessageTrackerEnabled::~UnAckedMessageTrackerEnabled() {
- if (timer_) {
- timer_->cancel();
+void UnAckedMessageTrackerEnabled::start() {
+ this->scheduleTimer();
+}
+
+void UnAckedMessageTrackerEnabled::close() {
+ std::lock_guard<std::mutex> acquire(mutexTimer_);
+ if (this->timer_) {
+ boost::system::error_code ec;
+ this->timer_.reset();
Review comment:
sorry, accidentally deleted
----------------------------------------------------------------
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] BewareMyPower commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735897363
Besides what I commented, a PR should not contain more than one tasks. I think
> - pulsar-client-cpp Consumer do AcknowledgeCumulative just clean up msgId, not <= msgId in UnAckedMessageTrackerEnabled::removeMessagesTill
> - potential crash caused by UnAckedMessageTrackerEnabled's timer(see issue like #8519)
are too tasks that are not so much related. Could you push another PR for the second issue? Though the tests may need changes once one of two PRs are merged.
And the change of `AckGroupingTrackerEnabled` is totally unrelated to this PR. By the way, currently the timer need no `reset` here because each time `AckGroupingTrackerEnabled::scheduleTimer` is called, the timer will be reset to a new instance. I think it's not a efficient way to schedule timer. The timer instance should be constructed in `start()` and destructed in `close()`. As well as the `UnAckedMessageTrackerEnabled`'s timer.
----------------------------------------------------------------
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] saosir commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735593147
Both `PartitionedConsumerImpl` and `ConsumerImpl` have member variable `unAckedMessageTrackerPtr_`, and `PartitionedConsumerImpl` is composed of `ConsumerImpl`. if the acknowledgement times out, will they send redeliverMessages repeatedly?
----------------------------------------------------------------
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] saosir commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735759684
@BewareMyPower `PartitionedConsumerImpl` internal consumer `ConsumerImpl::messageReceived` would be called first in `ClientConnection`, see
https://github.com/apache/pulsar/blob/102fa9de03509b86e47f58ab8e1c0dde2095da3b/pulsar-client-cpp/lib/ClientConnection.cc#L666-L669
and `ConsumerImpl#internalListener` will be called to notify with listener `PartitionedConsumerImpl::messageReceived`
----------------------------------------------------------------
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] saosir edited a comment on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir edited a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735759684
@BewareMyPower `PartitionedConsumerImpl` internal consumer `ConsumerImpl::messageReceived` would be called first in `ClientConnection`, see
https://github.com/apache/pulsar/blob/102fa9de03509b86e47f58ab8e1c0dde2095da3b/pulsar-client-cpp/lib/ClientConnection.cc#L666-L669
and `ConsumerImpl#internalListener` will be called and notify `PartitionedConsumerImpl` receive messge by listener of `ConsumerConfig`(here is `PartitionedConsumerImpl::messageReceived`)
----------------------------------------------------------------
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] saosir commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735632247
@BewareMyPower
Does `UnAckedMessageTrackerEnabled` have same problem like #8519 ?
----------------------------------------------------------------
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] saosir edited a comment on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir edited a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735759684
@BewareMyPower `PartitionedConsumerImpl` internal consumer `ConsumerImpl::messageReceived` would be called first in `ClientConnection`, see
https://github.com/apache/pulsar/blob/102fa9de03509b86e47f58ab8e1c0dde2095da3b/pulsar-client-cpp/lib/ClientConnection.cc#L666-L669
and `ConsumerImpl#internalListener` will be called and notify `PartitionedConsumerImpl` receive messge by listener `PartitionedConsumerImpl::messageReceived`
----------------------------------------------------------------
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 #8606: [C++] fix cpp client AcknowledgeCumulative
Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-732469965
@saosir Can you add a unit test that reproduces the issue? That will also help people to understand the problem.
----------------------------------------------------------------
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] saosir edited a comment on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir edited a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735759684
@BewareMyPower `PartitionedConsumerImpl` internal consumer `ConsumerImpl::messageReceived` would be called first in `ClientConnection`, see
https://github.com/apache/pulsar/blob/102fa9de03509b86e47f58ab8e1c0dde2095da3b/pulsar-client-cpp/lib/ClientConnection.cc#L666-L669
and `ConsumerImpl#internalListener` will be called and notify `PartitionedConsumerImpl` receive messge by config listener `PartitionedConsumerImpl::messageReceived`
----------------------------------------------------------------
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] saosir edited a comment on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir edited a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735593147
@BewareMyPower I have two questions here, can you help me answer them?
1. Both `PartitionedConsumerImpl` and `ConsumerImpl` have member variable `unAckedMessageTrackerPtr_` (class `UnAckedMessageTrackerEnabled`), and `PartitionedConsumerImpl` is composed of `ConsumerImpl`. if the acknowledgement times out, will they send redeliverMessages repeatedly?
2. in `UnAckedMessageTrackerEnabled`, param msg of `UnAckedMessageTrackerEnabled::add` contain `partition`, `ledgerId`, `entryId`,`batchIndex`. When ack timeout and do `redeliverUnacknowledgedMessages`, does it deduplicte based on ledger and entryId?
----------------------------------------------------------------
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] saosir edited a comment on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir edited a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735593147
@BewareMyPower I have two questions here, can you help me answer them?
1. Both `PartitionedConsumerImpl` and `ConsumerImpl` have member variable `unAckedMessageTrackerPtr_` (class `UnAckedMessageTrackerEnabled`), and `PartitionedConsumerImpl` is composed of `ConsumerImpl`. if the acknowledgement times out, will they send redeliverMessages repeatedly?
2. in `UnAckedMessageTrackerEnabled`, param msg of `UnAckedMessageTrackerEnabled::add` contain `partition`, `ledgerId`, `entryId`,`batchIndex`. When ack timeout and do `redeliverUnacknowledgedMessages`, does it deduplicte based on `ledgerId` and `entryId`? If not it will send repeat `ack(ledgerId, entryId)`
----------------------------------------------------------------
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] BewareMyPower edited a comment on pull request #8606: fix cpp client AcknowledgeCumulative
Posted by GitBox <gi...@apache.org>.
BewareMyPower edited a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-731595313
Could you explain why the last message id should not be removed? See https://github.com/apache/pulsar/blob/281163b26f3b842c0834ff46b481453cddea7860/pulsar-client-cpp/lib/ConsumerImpl.cc#L851-L854
After messages whose id is **<=** `messageId` are removed, the `messageId` would be added to `ackGroupingTrackerPtr_` to mark it as an acknowledged message though it's a pending state.
And see Java client's implementation: https://github.com/apache/pulsar/blob/281163b26f3b842c0834ff46b481453cddea7860/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java#L784-L786
https://github.com/apache/pulsar/blob/281163b26f3b842c0834ff46b481453cddea7860/pulsar-client/src/main/java/org/apache/pulsar/client/impl/UnAckedMessageTracker.java#L225-L239
The behaviors of Java and C++ client are the same.
----------------------------------------------------------------
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] saosir edited a comment on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir edited a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-736479664
> Thanks. Please update the associated PR description too.
>
> Also I think the tests could still be added without the refactor, like what `AckGroupingTrackerEnabled` tests did.
All is done
----------------------------------------------------------------
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] saosir commented on a change in pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir commented on a change in pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#discussion_r533853164
##########
File path: pulsar-client-cpp/tests/BasicEndToEndTest.cc
##########
@@ -3826,3 +3826,208 @@ TEST(BasicEndToEndTest, testAckGroupingTrackerEnabledCumulativeAck) {
ret = consumer.receive(msg, 1000);
ASSERT_EQ(ResultTimeout, ret) << "Received redundant message ID: " << msg.getMessageId();
}
+
+class UnAckedMessageTrackerEnabledMock : public UnAckedMessageTrackerEnabled {
+ public:
+ UnAckedMessageTrackerEnabledMock(long timeoutMs, const ClientImplPtr client, ConsumerImplBase &consumer)
+ : UnAckedMessageTrackerEnabled(timeoutMs, timeoutMs, client, consumer) {}
+ const long getUnAckedMessagesTimeoutMs() { return this->timeoutMs_; }
+ const long getTickDurationInMs() { return this->tickDurationInMs_; }
+ bool isEmpty() { return UnAckedMessageTrackerEnabled::isEmpty(); }
+ long size() { return UnAckedMessageTrackerEnabled::size(); }
+}; // class UnAckedMessageTrackerEnabledMock
+
+TEST(BasicEndToEndTest, testtUnAckedMessageTrackerDefaultBehavior) {
Review comment:
Do I need to resubmit the code to fix this problem?How about someone fix it when merge pull request?
----------------------------------------------------------------
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] saosir removed a comment on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir removed a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-750697945
/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] saosir edited a comment on pull request #8606: fix cpp client AcknowledgeCumulative
Posted by GitBox <gi...@apache.org>.
saosir edited a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-731891030
@BewareMyPower Sorry，I did not describe clearly. `UnAckedMessageTrackerEnabled::removeMessagesTill` should erase message whose id <= msgId in `messageIdPartitionMap`
see [https://github.com/apache/pulsar/pull/8606/files](https://github.com/apache/pulsar/pull/8606/files), the origin implementation of cpp is different from that of Java client. cpp client just earse `msgId`, not <= `msgId`
----------------------------------------------------------------
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] saosir commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-736317811
/pulsarbot run-cpp-tests
----------------------------------------------------------------
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] saosir commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-736479664
> Thanks. Please update the associated PR description too.
>
> Also I think the tests could still be added without the refactor, like what `AckGroupingTrackerEnabled` tests did.
all is done, but I don't know why the test case `BasicEndToEndTest.testUnAckedMessageTrackerEnabledIndividualAck` is fail. It is success run at my local machine :joy:
----------------------------------------------------------------
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] saosir commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-751346194
/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] saosir removed a comment on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir removed a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-736317811
/pulsarbot run-cpp-tests
----------------------------------------------------------------
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] saosir commented on pull request #8606: fix cpp client AcknowledgeCumulative
Posted by GitBox <gi...@apache.org>.
saosir commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-731891030
@BewareMyPower Sorry，I did not describe clearly. UnAckedMessageTrackerEnabled::removeMessagesTill should erase message whose id <= msgId in `messageIdPartitionMap`
see [https://github.com/apache/pulsar/pull/8606/files](https://github.com/apache/pulsar/pull/8606/files), the implementation of cpp is different from that of Java client. cpp client just earse `msgId`, not <= `msgId`
----------------------------------------------------------------
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] BewareMyPower commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735884386
@saosir You're right. I just missed here. The same message id would be added twice to unacked message tracker.
And just now I found the reason why my test showed redelivery only happened once for each partition. It's because the partitioned consumer's redelivery timer started a little earlier than the internal consumers' redelivery timer.
So nearly each time the partitioned consumer's redelivery callback was triggered first. It called all the internal consumers' redelivery callbacks which cleared the `UnAckedMessageTracker`.
Finally , it's time for the internal consumers to trigger the redelivery callback. But the redelivery wouldn't happen because the trackers were empty.
In conclusion, there's a race condition which is hard to happen if the number of partitions is not large. I think the tracker of `PartitionedConsumerImpl` is redundant.
----------------------------------------------------------------
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] saosir commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-750697945
/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] saosir edited a comment on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir edited a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735759684
@BewareMyPower `PartitionedConsumerImpl` internal consumer `ConsumerImpl::messageReceived` would be called first in `ClientConnection`, see
https://github.com/apache/pulsar/blob/102fa9de03509b86e47f58ab8e1c0dde2095da3b/pulsar-client-cpp/lib/ClientConnection.cc#L666-L669
and `ConsumerImpl#internalListener` will be called and notify `PartitionedConsumerImpl` receive messge by listener of `ConsumerConfiguration`(here is `PartitionedConsumerImpl::messageReceived`)
----------------------------------------------------------------
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] BewareMyPower commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-736581542
/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] BewareMyPower commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-736165357
Thanks. Please update the associated PR description too.
Also I think the tests could still be added without the refactor, like what `AckGroupingTrackerEnabled` tests did.
----------------------------------------------------------------
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] saosir commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-751366302
/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] BewareMyPower commented on a change in pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#discussion_r533469823
##########
File path: pulsar-client-cpp/tests/BasicEndToEndTest.cc
##########
@@ -3826,3 +3826,208 @@ TEST(BasicEndToEndTest, testAckGroupingTrackerEnabledCumulativeAck) {
ret = consumer.receive(msg, 1000);
ASSERT_EQ(ResultTimeout, ret) << "Received redundant message ID: " << msg.getMessageId();
}
+
+class UnAckedMessageTrackerEnabledMock : public UnAckedMessageTrackerEnabled {
+ public:
+ UnAckedMessageTrackerEnabledMock(long timeoutMs, const ClientImplPtr client, ConsumerImplBase &consumer)
+ : UnAckedMessageTrackerEnabled(timeoutMs, timeoutMs, client, consumer) {}
+ const long getUnAckedMessagesTimeoutMs() { return this->timeoutMs_; }
+ const long getTickDurationInMs() { return this->tickDurationInMs_; }
+ bool isEmpty() { return UnAckedMessageTrackerEnabled::isEmpty(); }
+ long size() { return UnAckedMessageTrackerEnabled::size(); }
+}; // class UnAckedMessageTrackerEnabledMock
+
+TEST(BasicEndToEndTest, testtUnAckedMessageTrackerDefaultBehavior) {
Review comment:
A typo: `testtUnAcked` -> `testUnAcked`
----------------------------------------------------------------
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 #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735524532
@BewareMyPower Can you review this pull request again?
----------------------------------------------------------------
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] saosir commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735531567
I have some other problem fix codes and test cases that need to be updated. I will submit the update later. @BewareMyPower
----------------------------------------------------------------
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] saosir removed a comment on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir removed a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-751346194
/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] BewareMyPower commented on a change in pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#discussion_r533854096
##########
File path: pulsar-client-cpp/tests/BasicEndToEndTest.cc
##########
@@ -3826,3 +3826,208 @@ TEST(BasicEndToEndTest, testAckGroupingTrackerEnabledCumulativeAck) {
ret = consumer.receive(msg, 1000);
ASSERT_EQ(ResultTimeout, ret) << "Received redundant message ID: " << msg.getMessageId();
}
+
+class UnAckedMessageTrackerEnabledMock : public UnAckedMessageTrackerEnabled {
+ public:
+ UnAckedMessageTrackerEnabledMock(long timeoutMs, const ClientImplPtr client, ConsumerImplBase &consumer)
+ : UnAckedMessageTrackerEnabled(timeoutMs, timeoutMs, client, consumer) {}
+ const long getUnAckedMessagesTimeoutMs() { return this->timeoutMs_; }
+ const long getTickDurationInMs() { return this->tickDurationInMs_; }
+ bool isEmpty() { return UnAckedMessageTrackerEnabled::isEmpty(); }
+ long size() { return UnAckedMessageTrackerEnabled::size(); }
+}; // class UnAckedMessageTrackerEnabledMock
+
+TEST(BasicEndToEndTest, testtUnAckedMessageTrackerDefaultBehavior) {
Review comment:
I think a committer could fix it when merge PR. So you needn't any changes. @jiazhai could you help take a look?
----------------------------------------------------------------
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] BewareMyPower commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-751462262
LGTM. @jiazhai Could you help take a look?
----------------------------------------------------------------
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] BewareMyPower commented on pull request #8606: [C++] fix cpp client AcknowledgeCumulative
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-732517613
@saosir OK, I see.
----------------------------------------------------------------
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] BewareMyPower commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-737209198
/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] sijie merged pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
sijie merged pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606
----------------------------------------------------------------
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 a change in pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#discussion_r550126159
##########
File path: pulsar-client-cpp/tests/BasicEndToEndTest.cc
##########
@@ -3826,3 +3826,208 @@ TEST(BasicEndToEndTest, testAckGroupingTrackerEnabledCumulativeAck) {
ret = consumer.receive(msg, 1000);
ASSERT_EQ(ResultTimeout, ret) << "Received redundant message ID: " << msg.getMessageId();
}
+
+class UnAckedMessageTrackerEnabledMock : public UnAckedMessageTrackerEnabled {
+ public:
+ UnAckedMessageTrackerEnabledMock(long timeoutMs, const ClientImplPtr client, ConsumerImplBase &consumer)
+ : UnAckedMessageTrackerEnabled(timeoutMs, timeoutMs, client, consumer) {}
+ const long getUnAckedMessagesTimeoutMs() { return this->timeoutMs_; }
+ const long getTickDurationInMs() { return this->tickDurationInMs_; }
+ bool isEmpty() { return UnAckedMessageTrackerEnabled::isEmpty(); }
+ long size() { return UnAckedMessageTrackerEnabled::size(); }
+}; // class UnAckedMessageTrackerEnabledMock
+
+TEST(BasicEndToEndTest, testtUnAckedMessageTrackerDefaultBehavior) {
Review comment:
@saosir Can you fix the typo?
----------------------------------------------------------------
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] saosir commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735580816
`PartitionedConsumerImpl` and `ConsumerImpl` have same member `unAckedMessageTrackerPtr_` (`PartitionedConsumerImpl` is composed of `ConsumerImpl`), if the acknowledgement times out, will they send redeliverMessages repeatedly？
----------------------------------------------------------------
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] BewareMyPower commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-750799119
/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] saosir commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-751371353
@BewareMyPower all is done
----------------------------------------------------------------
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] BewareMyPower commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735644945
> Does `UnAckedMessageTrackerEnabled` have same problem like #8519 ?
Yeah, `consumerReference_` may be invalid when the timer callback is called.
----------------------------------------------------------------
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 a change in pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#discussion_r550126159
##########
File path: pulsar-client-cpp/tests/BasicEndToEndTest.cc
##########
@@ -3826,3 +3826,208 @@ TEST(BasicEndToEndTest, testAckGroupingTrackerEnabledCumulativeAck) {
ret = consumer.receive(msg, 1000);
ASSERT_EQ(ResultTimeout, ret) << "Received redundant message ID: " << msg.getMessageId();
}
+
+class UnAckedMessageTrackerEnabledMock : public UnAckedMessageTrackerEnabled {
+ public:
+ UnAckedMessageTrackerEnabledMock(long timeoutMs, const ClientImplPtr client, ConsumerImplBase &consumer)
+ : UnAckedMessageTrackerEnabled(timeoutMs, timeoutMs, client, consumer) {}
+ const long getUnAckedMessagesTimeoutMs() { return this->timeoutMs_; }
+ const long getTickDurationInMs() { return this->tickDurationInMs_; }
+ bool isEmpty() { return UnAckedMessageTrackerEnabled::isEmpty(); }
+ long size() { return UnAckedMessageTrackerEnabled::size(); }
+}; // class UnAckedMessageTrackerEnabledMock
+
+TEST(BasicEndToEndTest, testtUnAckedMessageTrackerDefaultBehavior) {
Review comment:
@saosir Can you create a follow-up PR to fix the typo?
----------------------------------------------------------------
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] BewareMyPower commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-736947471
/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] saosir removed a comment on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
saosir removed a comment on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-750729469
@BewareMyPower the `CI - Integration` is failed, can you help?
----------------------------------------------------------------
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] BewareMyPower commented on pull request #8606: [C++] fix cpp client do AcknowledgeCumulative not clean up previous message
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-751245405
@saosir Yeah, your branch is far behind the master, it's better to rebase to latest master.
----------------------------------------------------------------
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] BewareMyPower commented on pull request #8606: fix cpp client AcknowledgeCumulative
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #8606:
URL: https://github.com/apache/pulsar/pull/8606#issuecomment-731595313
Could you explain why the last message id should not be removed? See https://github.com/apache/pulsar/blob/281163b26f3b842c0834ff46b481453cddea7860/pulsar-client-cpp/lib/ConsumerImpl.cc#L851-L854
After message ids which is **<=** `messageId` are removed, the `messageId` would be added to `ackGroupingTrackerPtr_` to mark it as an acknowledged message though it's a pending state.
And see Java client's implementation: https://github.com/apache/pulsar/blob/281163b26f3b842c0834ff46b481453cddea7860/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java#L784-L786
https://github.com/apache/pulsar/blob/281163b26f3b842c0834ff46b481453cddea7860/pulsar-client/src/main/java/org/apache/pulsar/client/impl/UnAckedMessageTracker.java#L225-L239
The behaviors of Java and C++ client are the same.
----------------------------------------------------------------
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