-
Notifications
You must be signed in to change notification settings - Fork 1k
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
fix: Consider topics created by join operations internal #8520
Conversation
final String internalTopic2 = applicationId | ||
+ "-KTABLE-FK-JOIN-SUBSCRIPTION-RESPONSE-0000000012-topic"; | ||
final String internalTopic3 = applicationId + "-something-repartition"; | ||
final String internalTopic4 = applicationId + "-someting-changelog"; |
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.
nit: typo in something
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.
3f3b945
to
0c9865e
Compare
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.
@Gerrrr Thank you for the PR!
I have a doubt about the expression to determine the internal topics.
&& (topicName.endsWith(KsqlConstants.STREAMS_CHANGELOG_TOPIC_SUFFIX) | ||
|| topicName.endsWith(KsqlConstants.STREAMS_REPARTITION_TOPIC_SUFFIX)); | ||
|| topicName.endsWith(KsqlConstants.STREAMS_REPARTITION_TOPIC_SUFFIX)) | ||
|| topicName.matches(KsqlConstants.STREAMS_JOIN_REGISTRATION_TOPIC_PATTERN) | ||
|| topicName.matches(KsqlConstants.STREAMS_JOIN_RESPONSE_TOPIC_PATTERN); |
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.
This expression does not seem completely correct. Currently it is A && (B || C) || D || E
. Shouldn't it be A && (B || C || D || E)
? Maybe that is also the reason you get a checkstyle error that you need to suppress.
If I am right, then we are missing some tests that check the negative case.
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.
Good catch! I fixed it and added checks for the negative cases in 9e2652e.
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.
LGTM!
Even though CI is broken, I think that it is ok to merge it because:
|
Description
Fixes #8456.
Ideally, we should probably rely on a standard predicate in KStreams, but given this issue is a release blocker, we can probably introduce this predicate to KStreams and update ksql's dependency in follow-up PRs.
Testing done
Reviewer checklist