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

Support seek operation on a multi-topics consumer #426

Merged
merged 6 commits into from
Jun 4, 2024

Conversation

BewareMyPower
Copy link
Contributor

Motivation

See apache/pulsar-client-python#213

Modifications

Add a new forEachValue overload that allows users to count the number of rest running tasks through SharedFuture to SynchronizedHashMap. Leverage this overload in seek operations when the argument is a timestamp, or a MessageId that represents earliest or latest. When the argument is a MessageId whose getTopicName() method returns a correct topic name, seek on the internal consumer of that topic.

Add testMultiTopicsSeekAll and testMultiTopicsSeekSingle to ConsumerSeekTest to cover these cases.

### Motivation

See apache/pulsar-client-python#213

### Modifications

Add a new `forEachValue` overload that allows users to count the number
of rest running tasks through `SharedFuture` to `SynchronizedHashMap`.
Leverage this overload in seek operations when the argument is a
timestamp, or a MessageId that represents earliest or latest. When the
argument is a MessageId whose `getTopicName()` method returns a correct
topic name, seek on the internal consumer of that topic.

Add `testMultiTopicsSeekAll` and `testMultiTopicsSeekSingle` to
`ConsumerSeekTest` to cover these cases.
@BewareMyPower BewareMyPower self-assigned this May 20, 2024
@BewareMyPower BewareMyPower added this to the 3.6.0 milestone May 20, 2024
Copy link
Member

@shibd shibd left a comment

Choose a reason for hiding this comment

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

Left some comments.

lib/MultiTopicsConsumerImpl.cc Show resolved Hide resolved
lib/MultiTopicsConsumerImpl.h Show resolved Hide resolved
lib/MultiTopicsConsumerImpl.cc Outdated Show resolved Hide resolved
lib/SynchronizedHashMap.h Show resolved Hide resolved
}
});
},
[callback] { callback(ResultOk); });
Copy link
Member

Choose a reason for hiding this comment

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

If there are no consumers, it means no cursor will be reset. Maybe we cannot return a ResultOk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I agree with you but here it just keeps the same behavior with the Java client. See the original implementation from apache/pulsar#7518

Copy link
Member

Choose a reason for hiding this comment

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

We can start a discussion, or we should add a WARN log on here.

Otherwise, when something goes wrong, it's hard to troubleshoot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Java client does not have a warn log here as well. You can start a discussion in dev ML. But it should not block this PR because this PR keeps the same behavior with the Java client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shibd I make the seek call fail with ResultOperationNotSupported with a error log now. PTAL again.

The discussion mail: https://lists.apache.org/thread/qrwvl1zshmdohphjtdyp9v98hdngxb30

@BewareMyPower
Copy link
Contributor Author

According to Zike's comment in the discussion in the mail list: https://lists.apache.org/thread/qrwvl1zshmdohphjtdyp9v98hdngxb30

So I reverted the change and only retained the test. PTAL again @shibd @RobertIndie

@BewareMyPower BewareMyPower merged commit 37bdf5b into apache:main Jun 4, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants