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

PIP-13-2/3: support regex based subscription #1165

Closed
wants to merge 6 commits into from

Conversation

zhaijack
Copy link
Contributor

@zhaijack zhaijack commented Feb 1, 2018

This change is based on work of #1103, which already contains the first 3 commits there. In this PR, the commits after the first 3 are for this sub-task.

Motivation

This is a second sub-task for pip-13, which would like to leverage the first task to support regex based subscription.

Modifications

  • add subscribe methods with regex pattern in PulsarClient and PulsarClientImpl.
  • add PatternTopicsConsumerImpl, which extends TopicsConsumerImpl.
  • add a binary proto command to get topics under a namespace.
  • add a test to verify.

Result

old methods behaviour not changed,
user could use new method to subscribe to topics based on regex pattern

@zhaijack zhaijack changed the title PIP-13-2: support regex based subscription PIP-13-2/3: support regex based subscription Feb 1, 2018
@zhaijack
Copy link
Contributor Author

zhaijack commented Feb 5, 2018

retest this please

@merlimat merlimat added the type/feature The PR added a new feature or issue requested a new feature label Feb 5, 2018
@merlimat merlimat added this to the 2.0.0-incubating milestone Feb 5, 2018
@@ -458,6 +470,9 @@ message BaseCommand {
REACHED_END_OF_TOPIC = 27;

SEEK = 28;

GET_TOPICS_OF_NAMESPACE = 29;
Copy link
Member

Choose a reason for hiding this comment

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

If I remembered correctly, we are bumping the protocol version in one of your other pull requests, if this change is going to be merged with that pull request in same release, I would suggest update the comment of that protocol version with these two new commands.

Otherwise, we have to bump the protocol version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. there was #1066 to get the last message id, which is marked as 1.22.

message CommandGetTopicsOfNamespace {
required uint64 request_id = 1;
required string property = 2;
required string cluster = 3;
Copy link
Member

Choose a reason for hiding this comment

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

If this change is going to be merged at 2.0, there might some changes to be coordinated here, because cluster might be removed at 2.0, so that we don't need to include cluster here.

this is just a FYI. you don't need to change at this moment. /cc @merlimat

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.

@sijie
Copy link
Member

sijie commented Feb 5, 2018

@zhaijack the overall change looks good. but I will take a closer review once #1103 is in.

@zhaijack
Copy link
Contributor Author

would like to close this one and open a new PR.

@zhaijack zhaijack closed this Feb 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants