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.

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

fix: ack timeout in pulsar cpp client when subscribing to regex topic #3879

Merged
merged 7 commits into from
Mar 24, 2019

Conversation

jerrypeng
Copy link
Contributor

Motivation

When subscribing to a regex in cpp/python client and using a message listener, ack timeout does work and unacked messages are not delivered.

This is because the unack message tracker is not call in one place

@jerrypeng jerrypeng self-assigned this Mar 21, 2019
@jerrypeng jerrypeng added the type/bug The PR fixed a bug or issue reported a bug label Mar 21, 2019
@jerrypeng jerrypeng added this to the 2.4.0 milestone Mar 21, 2019
Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

LGTM. Can you add a test case for this regex topic and ack timeout?

@jerrypeng
Copy link
Contributor Author

@merlimat i have added a test

@@ -2940,3 +2940,50 @@ TEST(BasicEndToEndTest, testPreventDupConsumersAllowSameSubForDifferentTopics) {
// consumer C should be a different instance from A and B and should be with open state.
ASSERT_EQ(ResultOk, consumerC.close());
}

static long regexTestMessagesReceived = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

This gets updated from one thread and read from another. We should use std::atomic<long>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can but does it really matter since its only one writer? Also we doing the same thing in a couple places:
https://github.com/apache/pulsar/blob/master/pulsar-client-cpp/tests/BasicEndToEndTest.cc#L47

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the number will be correct because of the single writer, but the reader is technically not guaranteed to see the latest value. Though, yes, with sleep and retries it would settle anyway.

if (regexTestMessagesReceived >= 10 * 2) {
break;
}
usleep(500000);
Copy link
Contributor

Choose a reason for hiding this comment

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

unistd.h is only posix specific. std::this_thread::sleep is the preferred c++ 11 way to sleep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok will update

@jerrypeng
Copy link
Contributor Author

rerun java8 tests

@jerrypeng
Copy link
Contributor Author

rerun java8 tests
rerun integration tests

@jerrypeng
Copy link
Contributor Author

rerun java8 tests

@jiazhai
Copy link
Member

jiazhai commented Mar 24, 2019

rerun java8 tests

@merlimat merlimat merged commit 16e3b12 into apache:master Mar 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants