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

KAFKA-15561 [2/N]: Background event and subscription state changes for RE2J pattern #17918

Merged
merged 4 commits into from
Nov 26, 2024

Conversation

lianetm
Copy link
Contributor

@lianetm lianetm commented Nov 22, 2024

This PR includes:

  • new background event to support subscription to RE2J regex.
  • processing the new event to update the subscription state (so this includes changes in the SubscriptionState to support the new subscription type)

@lianetm
Copy link
Contributor Author

lianetm commented Nov 22, 2024

FYI, next PR will contain the integration with the HB and integration tests.

@lianetm lianetm requested a review from dajac November 22, 2024 21:17
@@ -108,13 +112,21 @@ private enum SubscriptionType {
public synchronized String toString() {
return "SubscriptionState{" +
"type=" + subscriptionType +
", subscribedPattern=" + subscribedPattern +
", subscribedPattern=" + subscribedPatternInUse() +
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'm intentionally avoiding differentiating the 2 patterns output thinking that it will help have a smooth transition when we end up deprecating/removing the old pattern in the future. Makes sense?

@lianetm
Copy link
Contributor Author

lianetm commented Nov 22, 2024

Comment regarding the mixed subscription error msg in SubscriptionState.

"Subscription to topics, partitions and pattern are mutually exclusive";

It's truly not clearly stating that we don't allow pattern and re2j pattern together (we don't), but I intentionally didn't change it wondering if it's more of a hassle given that this is expected to be a transition period until we deprecate (eventually remove) the old regex, and then the current msg will be fully accurate as it is. Let me know what you think.

Copy link
Contributor

@dajac dajac left a comment

Choose a reason for hiding this comment

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

LGTM

@lianetm lianetm merged commit 0b081fc into apache:trunk Nov 26, 2024
8 checks passed
chiacyu pushed a commit to chiacyu/kafka that referenced this pull request Nov 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants