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-6071: Use ZookeeperClient in LogManager #4089

Closed
wants to merge 3 commits into from

Conversation

omkreddy
Copy link
Contributor

No description provided.

Copy link
Contributor

@junrao junrao left a comment

Choose a reason for hiding this comment

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

@omkreddy : Thanks for the patch. Just a minor comment below.

val topicConfigs = AdminUtils.fetchAllTopicConfigs(zkUtils).map { case (topic, configs) =>
topic -> LogConfig.fromProps(defaultProps, configs)
}
val (topicConfigs, failed) = zkUtils.getLogConfigs(zkUtils.getAllTopicsInCluster, defaultProps)
Copy link
Contributor

Choose a reason for hiding this comment

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

The current behavior is to throw an exception if any ZK operation fails. We probably want to preserve this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@junrao Thanks for the review
I have included IllegalArgumentException if any failed topics while fetching configs. Updated the PR.
Not sure if IllegalArgumentException is not appropriate here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@omkreddy : Failed includes exception. We can just throw the one in the first failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@junrao Updated the PR.

@junrao
Copy link
Contributor

junrao commented Oct 20, 2017

@omkreddy : Thanks for the patch. LGTM

@asfgit asfgit closed this in fdbd4d6 Oct 20, 2017
jeqo pushed a commit to jeqo/kafka that referenced this pull request Nov 16, 2017
Author: Manikumar Reddy <manikumar.reddy@gmail.com>

Reviewers: Jun Rao <junrao@gmail.com>

Closes apache#4089 from omkreddy/KAFKA-6071-ZK-LOGMANAGER
@omkreddy omkreddy deleted the KAFKA-6071-ZK-LOGMANAGER branch July 3, 2018 15:42
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.

2 participants