Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[issue 4589] Fix redelivered message logic of partition topic #4653

Merged
merged 4 commits into from
Jul 19, 2019
Merged

[issue 4589] Fix redelivered message logic of partition topic #4653

merged 4 commits into from
Jul 19, 2019

Conversation

wolfstudy
Copy link
Member

@wolfstudy wolfstudy commented Jul 1, 2019 •
edited by jiazhai
Loading

Fixes #4589

Motivation

When using Partition-topic, the logic of redeliver messages will not be triggered when the time of ackTimeout arrives.

This is because the unAckedMessageTrackerPtr_->add(msg.getMessageId()) is not call in the listener handling of partitioned topic in cpp code

@wolfstudy
Copy link
Member Author

@cckellogg @merlimat @sijie @jiazhai PTAL, thanks

@wolfstudy
Copy link
Member Author

run cpp tests

@wolfstudy wolfstudy mentioned this pull request Jul 2, 2019
Closed
@sijie sijie added component/c++ area/client type/bug The PR fixed a bug or issue reported a bug labels Jul 3, 2019
@sijie sijie added this to the 2.4.1 milestone Jul 3, 2019
jiazhai
jiazhai reviewed Jul 3, 2019
View reviewed changes
pulsar-client-cpp/lib/PartitionedConsumerImpl.cc
@@ -365,6 +365,7 @@ void PartitionedConsumerImpl::messageReceived(Consumer consumer, const Message&
}
messages_.push(msg);
if (messageListener_) {
unAckedMessageTrackerPtr_->add(msg.getMessageId());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work. Seems this is only related with (messageListener_ != null), And not covered by the test case added.

@sijie
Copy link
Member

sijie commented Jul 7, 2019

@wolfstudy can you please take a look at @jiazhai 's comment?

@sijie
Copy link
Member

sijie commented Jul 10, 2019

ping @wolfstudy @jiazhai

@wolfstudy
Copy link
Member Author

@jiazhai PTAL again, thanks.

@wolfstudy wolfstudy changed the title [issue 4589] Fix redelivered message logic of partition topic [WIP][issue 4589] Fix redelivered message logic of partition topic Jul 12, 2019
@jiazhai
Copy link
Member

jiazhai commented Jul 12, 2019 •
edited
Loading

@wolfstudy Thanks for the work. This fix is needed, If go client set listener when create partitionedConsumer, this is the right fix.

I would suggest add the test for consumer listener with this fix.

@wolfstudy wolfstudy changed the title [WIP][issue 4589] Fix redelivered message logic of partition topic [issue 4589] Fix redelivered message logic of partition topic Jul 12, 2019
@wolfstudy
Copy link
Member Author

@wolfstudy Thanks for the work. This fix is needed, If go client set listener when create partitionedConsumer, this is the right fix.

I would suggest add the test for consumer listener with this fix.

thanks @jiazhai help, in cgo-client the consumer listener have be set.

@wolfstudy
[issue 4589] Fix delivered message logic of partition topic
ccd1d5b 
Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>
@wolfstudy
format code
0146f08 
Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>
@wolfstudy
add acktimeout test
6d1b08e 
Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>
@wolfstudy
address comments
41e71a8 
Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>
@wolfstudy wolfstudy force-pushed the xiaolong/cpp-client-partition-topic-not-receive-messages branch from 1a3b154 to 41e71a8 Compare July 18, 2019 12:49
@wolfstudy
Copy link
Member Author

run java8 tests
run cpp tests
run integration tests

1 similar comment
@wolfstudy
Copy link
Member Author

run java8 tests
run cpp tests
run integration tests

@wolfstudy
Copy link
Member Author

run java8 tests

@jiazhai jiazhai merged commit cc5f25b into apache:master Jul 19, 2019
easyfan pushed a commit to easyfan/pulsar that referenced this pull request Jul 26, 2019
@wolfstudy @easyfan
[issue 4589] Fix redelivered message logic of partition topic (apache…
88bcb79 
…#4653)

Fixes apache#4589

Motivation
When using Partition-topic, the logic of redeliver messages will not be triggered when the time of ackTimeout arrives.

This is because the unAckedMessageTrackerPtr_->add(msg.getMessageId()) is not call in the listener handling of partitioned topic in cpp code
jiazhai pushed a commit that referenced this pull request Aug 28, 2019
@wolfstudy @jiazhai
[issue 4589] Fix redelivered message logic of partition topic (#4653)
04e11bb 
Fixes #4589

Motivation
When using Partition-topic, the logic of redeliver messages will not be triggered when the time of ackTimeout arrives.

This is because the unAckedMessageTrackerPtr_->add(msg.getMessageId()) is not call in the listener handling of partitioned topic in cpp code
(cherry picked from commit cc5f25b)
@stevenwangnarvar stevenwangnarvar mentioned this pull request Sep 20, 2019
Closed
@sijie sijie mentioned this pull request Dec 27, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sijie sijie sijie approved these changes

@jiazhai jiazhai jiazhai approved these changes

Assignees

@wolfstudy wolfstudy

Labels
area/client type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Milestone
2.4.1
Development

Successfully merging this pull request may close these issues.

In Go client 2.3.2, can't get re-delivered message after add AckTimeout in ConsumerOptions
3 participants
@wolfstudy @sijie @jiazhai