-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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-5232 Fix Log.parseTopicPartitionName to take into account dot character in topic names of deleted topics #3043
Conversation
By the way, this issue also affects trunk. I don't remember what the process is for PR submission - is a separate PR against trunk needed? |
Thanks for the PR. Yes, we generally submit PRs for trunk and cherry-pick if it applies cleanly. Otherwise, separate PRs are needed. |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
edit: I was wrong regex checks out so I removed my comment |
else dirName | ||
|
||
if (!name.contains('-')) { |
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 trunk we also have if (dirName.endsWith(DeleteDirSuffix) && !dirName.matches("^(\\S+)-(\\S+)\\.(\\S+)" + DeleteDirSuffix))
. We need to double-check if this check is still needed given that.
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.
@ijuma, I had a look at the code in trunk and as you note, this additional check won't be necessary there. I will raise a separate PR for trunk which won't include this additional check.
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. Maybe we should cherry-pick the commit that added that check in trunk to 0.10.2 as well.
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 point. That commit which brought in that regex check, in trunk, was self contained. So I have cherry-picked it and included it in this PR.
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
…aking into account the potential presence of '.' character in topic names while looking for topics that are marked for deletion
@ijuma, done. PR has been updated. Given the nature of changes, please review once more, before merging. |
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, will merge after the tests pass.
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
…with a period in the name This issue would only be triggered if a broker was restarted while deletion was still taking place. Included a few minor improvements to that method and its tests. A separate PR was submitted for 0.10.2 as the trunk change did not merge cleanly. Author: Jaikiran Pai <jaikiran.pai@gmail.com> Reviewers: Ryan Pridgeon <ryan.n.pridgeon@gmail.com>, Ismael Juma <ismael@juma.me.uk> Closes #3043 from jaikiran/KAFKA-5232
Refer to this link for build results (access rights to CI server needed): |
@jaikiran, can you please close this PR since it's been merged? |
Closing. |
The commit here contains a fix and testcase for the issue reported in https://issues.apache.org/jira/browse/KAFKA-5232, where if a topic marked for deletion, has a
.
character in its name then the Kafka broker fails to start and keeps shutting down