Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
KAFKA-15561: Client support for new SubscriptionPattern based subscription #15188
KAFKA-15561: Client support for new SubscriptionPattern based subscription #15188
Changes from 13 commits
61037a8
423b6a2
a90da13
4df71e9
819c5ec
8e8e6c6
e6a1bd5
38c08d3
20a1835
1f718ff
366bc72
5841899
c66c474
a48a84d
df300c7
aa0b1d1
bbf604d
53fb53d
0d6caa1
695b58b
b5eb9c3
ec26dcb
443739a
d035c7d
3d3ff82
0f8b7d3
d228f6d
969e0a6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should be "shorthand".
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.
It would be very easy to adapt the implementation of
subscribe(Pattern pattern, ConsumerRebalanceListener callback)
for this rather than leaving it blank.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.
Agreed. This is necessary for full coverage of the client-side changes.
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.
@AndrewJSchofield, just a question, why do we need to adapt the implementation of
subscribe(Pattern pattern, ConsumerRebalanceListener callback)
while theSubscriptionPattern
is to be resolved on the broker, not locally likePattern.
If we go that route theMockConsumer
won't really simulate how theAsyncConsumer
worksThere 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.
I don't mind about the details of the implementation here, but I expect you do need a mocked implemented of this new method in order to complete the PR.
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.
And here.
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.
We've made a lot of effort to move any mutation of the
SubscriptionState
object to the background thread. Is there any reason we should not follow the same pattern (pun intended) here? If there's a good reason not to, we definitely should add some documentary comments to that effect.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.
What about having a class that contains either a
SubscriptionPattern
or aPattern
?Something like :
In such a way we would encapsulate the code that ensures that there is only of both set.
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.
I think it is a bit overkill to have an abstraction like this, the Pattern and SubscriptionPattern are already abstractions for the underneath regex string. What do you guys think @lianetm @dajac?
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.
On second thought, we should adopt this class.
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.
Nvm, I think adding this class would be a bit much when it only encapsulate the logic to check whether pattern or subscriptionPattern is set
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.
I agree with @cadonna. It seems very odd to me having
subscribedPattern
andsubscriptionPattern
. I understand that there are old patterns and new patterns, but really they are achieving the same thing. Hiding the difference insidesubscribedPattern
seems sensible to me.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.
@AndrewJSchofield I understand, will change the
subscribedPattern
accordinglyThere 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.
Nit: I find it's better to avoid changes in unrelated files, even if minor
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.
My bad
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.
Could you please revert this change?