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-14446: code style improvements for broker-to-controller forwarding #13001

Merged
merged 7 commits into from
Dec 21, 2022

Conversation

akhileshchg
Copy link
Contributor

…forwarding patch

Original patch: https://github.com/apache/kafka/pull/12961/files#

@cmccabe cmccabe changed the title KAFKA-14446: Follow up to the comments unaddressed on Zk brokers API … KAFKA-14446: code style improvements for broker-to-controller forwarding Dec 16, 2022
@@ -703,8 +703,8 @@ class KafkaServer(
case None =>
info(s"Broker registration for controller $controllerId is not available in the metadata cache")
}
case None =>
info("No controller present in the metadata cache")
case _ =>
Copy link
Contributor

Choose a reason for hiding this comment

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

this one confuses me. Isn't case None clearer than case _ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The condition covers more than None. It includes Some(controller: KRaftCachedControllerId) as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me add more explicit condition to prevent this confusion.

Copy link
Contributor

@cmccabe cmccabe left a comment

Choose a reason for hiding this comment

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

LGTM when comments are addressed

Copy link
Contributor

@cmccabe cmccabe left a comment

Choose a reason for hiding this comment

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

LGTM

@cmccabe cmccabe merged commit 755e04a into apache:trunk Dec 21, 2022
cmccabe pushed a commit that referenced this pull request Jan 5, 2023
…ing (#13001)

Reviewers: Colin P. McCabe <cmccabe@apache.org>, Ismael Juma <ismael@juma.me.uk>, David Arthur <mumrah@gmail.com>
guozhangwang pushed a commit to guozhangwang/kafka that referenced this pull request Jan 25, 2023
…ing (apache#13001)

Reviewers: Colin P. McCabe <cmccabe@apache.org>, Ismael Juma <ismael@juma.me.uk>, David Arthur <mumrah@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants