-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Conversation
5e54765
to
8d6a58f
Compare
8d6a58f
to
2104ceb
Compare
2104ceb
to
ff4bc1a
Compare
retest this please |
@@ -458,6 +470,9 @@ message BaseCommand { | |||
REACHED_END_OF_TOPIC = 27; | |||
|
|||
SEEK = 28; | |||
|
|||
GET_TOPICS_OF_NAMESPACE = 29; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
would like to close this one and open a new PR. |
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
PulsarClient
andPulsarClientImpl
.PatternTopicsConsumerImpl
, which extendsTopicsConsumerImpl
.Result
old methods behaviour not changed,
user could use new method to subscribe to topics based on regex pattern